* Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
@ 2022-03-18 10:08 kernel test robot
2022-03-20 15:17 ` kernel test robot
0 siblings, 1 reply; 8+ messages in thread
From: kernel test robot @ 2022-03-18 10:08 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 16249 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220317053907.164160-4-david@fromorbit.com>
References: <20220317053907.164160-4-david@fromorbit.com>
TO: Dave Chinner <david@fromorbit.com>
Hi Dave,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on v5.17-rc8 next-20220317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dave-Chinner/xfs-log-recovery-fixes/20220317-141849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
:::::: branch date: 28 hours ago
:::::: commit date: 28 hours ago
config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220318/202203181702.8MiQ7z1k-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a6ec1e3d798f8eab43fb3a91028c6ab04e115fcb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/29e90a4845ecee7dcc9d1e1af7ab6bb2231cfb1a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dave-Chinner/xfs-log-recovery-fixes/20220317-141849
git checkout 29e90a4845ecee7dcc9d1e1af7ab6bb2231cfb1a
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
^
include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
(cond) ? \
^
fs/xfs/xfs_trans_ail.c:459:3: note: Taking false branch
if (lip)
^
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
fs/xfs/xfs_trans_ail.c:470:7: note: 'lip' is non-null
if (!lip)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
fs/xfs/xfs_trans_ail.c:470:2: note: '?' condition is false
if (!lip)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
fs/xfs/xfs_trans_ail.c:470:7: note: 'lip' is non-null
if (!lip)
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
fs/xfs/xfs_trans_ail.c:470:2: note: '?' condition is false
if (!lip)
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:69: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
(cond) ? \
^
fs/xfs/xfs_trans_ail.c:470:2: note: Taking false branch
if (!lip)
^
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
fs/xfs/xfs_trans_ail.c:473:2: note: Loop condition is false. Exiting loop
XFS_STATS_INC(mp, xs_push_ail);
^
fs/xfs/xfs_stats.h:165:2: note: expanded from macro 'XFS_STATS_INC'
per_cpu_ptr(xfsstats.xs_stats, current_cpu())->s.v++; \
^
include/linux/percpu-defs.h:263:47: note: expanded from macro 'per_cpu_ptr'
#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); })
^
include/linux/percpu-defs.h:259:2: note: expanded from macro 'VERIFY_PERCPU_PTR'
__verify_pcpu_ptr(__p); \
^
include/linux/percpu-defs.h:217:37: note: expanded from macro '__verify_pcpu_ptr'
#define __verify_pcpu_ptr(ptr) \
^
fs/xfs/xfs_trans_ail.c:473:2: note: Loop condition is false. Exiting loop
XFS_STATS_INC(mp, xs_push_ail);
^
fs/xfs/xfs_stats.h:166:2: note: expanded from macro 'XFS_STATS_INC'
per_cpu_ptr(mp->m_stats.xs_stats, current_cpu())->s.v++; \
^
include/linux/percpu-defs.h:263:47: note: expanded from macro 'per_cpu_ptr'
#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); })
^
include/linux/percpu-defs.h:259:2: note: expanded from macro 'VERIFY_PERCPU_PTR'
__verify_pcpu_ptr(__p); \
^
include/linux/percpu-defs.h:217:37: note: expanded from macro '__verify_pcpu_ptr'
#define __verify_pcpu_ptr(ptr) \
^
fs/xfs/xfs_trans_ail.c:473:2: note: Loop condition is false. Exiting loop
XFS_STATS_INC(mp, xs_push_ail);
^
fs/xfs/xfs_stats.h:163:34: note: expanded from macro 'XFS_STATS_INC'
#define XFS_STATS_INC(mp, v) \
^
fs/xfs/xfs_trans_ail.c:476:10: note: 2nd function call argument is an uninitialized value
while ((XFS_LSN_CMP(lip->li_lsn, target) <= 0)) {
^
fs/xfs/xfs_log.h:91:26: note: expanded from macro 'XFS_LSN_CMP'
#define XFS_LSN_CMP(x,y) _lsn_cmp(x,y)
^ ~
>> fs/xfs/xfs_trans_ail.c:737:10: warning: Although the value stored to 'lip' is used in the enclosing expression, the value is never actually read from 'lip' [clang-analyzer-deadcode.DeadStores]
while ((lip = xfs_ail_max(ailp)) != NULL) {
^ ~~~~~~~~~~~~~~~~~
fs/xfs/xfs_trans_ail.c:737:10: note: Although the value stored to 'lip' is used in the enclosing expression, the value is never actually read from 'lip'
while ((lip = xfs_ail_max(ailp)) != NULL) {
^ ~~~~~~~~~~~~~~~~~
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning generated.
sound/drivers/opl4/opl4_mixer.c:74:2: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
strcat(card->mixername, ",OPL4");
^~~~~~
sound/drivers/opl4/opl4_mixer.c:74:2: note: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119
strcat(card->mixername, ",OPL4");
^~~~~~
2 warnings generated.
sound/drivers/opl4/opl4_synth.c:444:18: warning: The result of the left shift is undefined because the left operand is negative [clang-analyzer-core.UndefinedBinaryOperatorResult]
(octave << 4) | ((pitch >> 7) & OPL4_F_NUMBER_HIGH_MASK));
^
sound/drivers/opl4/opl4_synth.c:589:2: note: Control jumps to 'case 128:' at line 618
switch (type) {
^
sound/drivers/opl4/opl4_synth.c:619:3: note: Calling 'snd_opl4_do_for_channel'
snd_opl4_do_for_channel(opl4, chan, snd_opl4_update_pitch);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/drivers/opl4/opl4_synth.c:340:2: note: Loop condition is false. Exiting loop
spin_lock_irqsave(&opl4->reg_lock, flags);
^
include/linux/spinlock.h:379:2: note: expanded from macro 'spin_lock_irqsave'
raw_spin_lock_irqsave(spinlock_check(lock), flags); \
^
include/linux/spinlock.h:240:2: note: expanded from macro 'raw_spin_lock_irqsave'
do { \
^
sound/drivers/opl4/opl4_synth.c:340:2: note: Loop condition is false. Exiting loop
spin_lock_irqsave(&opl4->reg_lock, flags);
^
include/linux/spinlock.h:377:43: note: expanded from macro 'spin_lock_irqsave'
#define spin_lock_irqsave(lock, flags) \
^
sound/drivers/opl4/opl4_synth.c:341:2: note: Loop condition is true. Entering loop body
for (i = 0; i < OPL4_MAX_VOICES; i++) {
^
sound/drivers/opl4/opl4_synth.c:343:7: note: Assuming 'chan' is equal to field 'chan'
if (voice->chan == chan) {
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
sound/drivers/opl4/opl4_synth.c:343:3: note: '?' condition is false
if (voice->chan == chan) {
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:31: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
sound/drivers/opl4/opl4_synth.c:343:22: note: 'chan' is equal to field 'chan'
if (voice->chan == chan) {
^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
sound/drivers/opl4/opl4_synth.c:343:3: note: '?' condition is true
if (voice->chan == chan) {
^
include/linux/compiler.h:56:28: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
include/linux/compiler.h:58:69: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^
include/linux/compiler.h:69:2: note: expanded from macro '__trace_if_value'
(cond) ? \
^
sound/drivers/opl4/opl4_synth.c:343:3: note: Taking true branch
if (voice->chan == chan) {
^
include/linux/compiler.h:56:23: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^
sound/drivers/opl4/opl4_synth.c:344:4: note: Calling 'snd_opl4_update_pitch'
func(opl4, voice);
^~~~~~~~~~~~~~~~~
sound/drivers/opl4/opl4_synth.c:425:9: note: Assuming field 'drum_channel' is not equal to 0
note = chan->drum_channel ? 60 : voice->note;
^~~~~~~~~~~~~~~~~~
sound/drivers/opl4/opl4_synth.c:425:9: note: '?' condition is true
sound/drivers/opl4/opl4_synth.c:432:13: note: Field 'drum_channel' is not equal to 0
if (!chan->drum_channel)
^
vim +737 fs/xfs/xfs_trans_ail.c
fd074841cfe01b Dave Chinner 2011-04-08 725
211e4d434bd737 Christoph Hellwig 2012-04-23 726 /*
211e4d434bd737 Christoph Hellwig 2012-04-23 727 * Push out all items in the AIL immediately and wait until the AIL is empty.
211e4d434bd737 Christoph Hellwig 2012-04-23 728 */
211e4d434bd737 Christoph Hellwig 2012-04-23 729 void
211e4d434bd737 Christoph Hellwig 2012-04-23 730 xfs_ail_push_all_sync(
211e4d434bd737 Christoph Hellwig 2012-04-23 731 struct xfs_ail *ailp)
211e4d434bd737 Christoph Hellwig 2012-04-23 732 {
211e4d434bd737 Christoph Hellwig 2012-04-23 733 struct xfs_log_item *lip;
211e4d434bd737 Christoph Hellwig 2012-04-23 734 DEFINE_WAIT(wait);
211e4d434bd737 Christoph Hellwig 2012-04-23 735
57e809561118a4 Matthew Wilcox 2018-03-07 736 spin_lock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 @737 while ((lip = xfs_ail_max(ailp)) != NULL) {
57e809561118a4 Matthew Wilcox 2018-03-07 738 prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
57e809561118a4 Matthew Wilcox 2018-03-07 739 wake_up_process(ailp->ail_task);
57e809561118a4 Matthew Wilcox 2018-03-07 740 spin_unlock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 741 schedule();
57e809561118a4 Matthew Wilcox 2018-03-07 742 spin_lock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 743 }
57e809561118a4 Matthew Wilcox 2018-03-07 744 spin_unlock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 745
57e809561118a4 Matthew Wilcox 2018-03-07 746 finish_wait(&ailp->ail_empty, &wait);
211e4d434bd737 Christoph Hellwig 2012-04-23 747 }
211e4d434bd737 Christoph Hellwig 2012-04-23 748
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
2022-03-18 10:08 [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates kernel test robot
@ 2022-03-20 15:17 ` kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2022-03-20 15:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: llvm, kbuild-all
Hi Dave,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on v5.17-rc8 next-20220317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dave-Chinner/xfs-log-recovery-fixes/20220317-141849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220318/202203181702.8MiQ7z1k-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a6ec1e3d798f8eab43fb3a91028c6ab04e115fcb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/29e90a4845ecee7dcc9d1e1af7ab6bb2231cfb1a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dave-Chinner/xfs-log-recovery-fixes/20220317-141849
git checkout 29e90a4845ecee7dcc9d1e1af7ab6bb2231cfb1a
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=i386 clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <yujie.liu@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
>> fs/xfs/xfs_trans_ail.c:737:10: warning: Although the value stored to 'lip' is used in the enclosing expression, the value is never actually read from 'lip' [clang-analyzer-deadcode.DeadStores]
while ((lip = xfs_ail_max(ailp)) != NULL) {
^ ~~~~~~~~~~~~~~~~~
vim +737 fs/xfs/xfs_trans_ail.c
fd074841cfe01b Dave Chinner 2011-04-08 725
211e4d434bd737 Christoph Hellwig 2012-04-23 726 /*
211e4d434bd737 Christoph Hellwig 2012-04-23 727 * Push out all items in the AIL immediately and wait until the AIL is empty.
211e4d434bd737 Christoph Hellwig 2012-04-23 728 */
211e4d434bd737 Christoph Hellwig 2012-04-23 729 void
211e4d434bd737 Christoph Hellwig 2012-04-23 730 xfs_ail_push_all_sync(
211e4d434bd737 Christoph Hellwig 2012-04-23 731 struct xfs_ail *ailp)
211e4d434bd737 Christoph Hellwig 2012-04-23 732 {
211e4d434bd737 Christoph Hellwig 2012-04-23 @733 struct xfs_log_item *lip;
211e4d434bd737 Christoph Hellwig 2012-04-23 734 DEFINE_WAIT(wait);
211e4d434bd737 Christoph Hellwig 2012-04-23 735
57e809561118a4 Matthew Wilcox 2018-03-07 736 spin_lock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 @737 while ((lip = xfs_ail_max(ailp)) != NULL) {
57e809561118a4 Matthew Wilcox 2018-03-07 738 prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
57e809561118a4 Matthew Wilcox 2018-03-07 739 wake_up_process(ailp->ail_task);
57e809561118a4 Matthew Wilcox 2018-03-07 740 spin_unlock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 741 schedule();
57e809561118a4 Matthew Wilcox 2018-03-07 742 spin_lock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 743 }
57e809561118a4 Matthew Wilcox 2018-03-07 744 spin_unlock(&ailp->ail_lock);
211e4d434bd737 Christoph Hellwig 2012-04-23 745
57e809561118a4 Matthew Wilcox 2018-03-07 746 finish_wait(&ailp->ail_empty, &wait);
211e4d434bd737 Christoph Hellwig 2012-04-23 747 }
211e4d434bd737 Christoph Hellwig 2012-04-23 748
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
@ 2022-03-17 14:30 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2022-03-17 14:30 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 15245 bytes --]
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220317053907.164160-4-david@fromorbit.com>
References: <20220317053907.164160-4-david@fromorbit.com>
TO: Dave Chinner <david@fromorbit.com>
TO: linux-xfs(a)vger.kernel.org
Hi Dave,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xfs-linux/for-next]
[also build test WARNING on v5.17-rc8 next-20220316]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dave-Chinner/xfs-log-recovery-fixes/20220317-141849
base: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
:::::: branch date: 8 hours ago
:::::: commit date: 8 hours ago
config: parisc-randconfig-m031-20220317 (https://download.01.org/0day-ci/archive/20220317/202203172212.pRLbx3jA-lkp(a)intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
fs/xfs/xfs_trans_ail.c:476 xfsaild_push() error: uninitialized symbol 'target'.
vim +/target +476 fs/xfs/xfs_trans_ail.c
7f4d01f36a3ac1 Brian Foster 2017-08-08 416
0030807c66f058 Christoph Hellwig 2011-10-11 417 static long
0030807c66f058 Christoph Hellwig 2011-10-11 418 xfsaild_push(
0030807c66f058 Christoph Hellwig 2011-10-11 419 struct xfs_ail *ailp)
249a8c1124653f David Chinner 2008-02-05 420 {
57e809561118a4 Matthew Wilcox 2018-03-07 421 xfs_mount_t *mp = ailp->ail_mount;
af3e40228fb2db Dave Chinner 2011-07-18 422 struct xfs_ail_cursor cur;
efe2330fdc246a Christoph Hellwig 2019-06-28 423 struct xfs_log_item *lip;
9e7004e741de0b Dave Chinner 2011-05-06 424 xfs_lsn_t lsn;
fe0da767311933 Dave Chinner 2011-05-06 425 xfs_lsn_t target;
43ff2122e6492b Christoph Hellwig 2012-04-23 426 long tout;
9e7004e741de0b Dave Chinner 2011-05-06 427 int stuck = 0;
43ff2122e6492b Christoph Hellwig 2012-04-23 428 int flushing = 0;
9e7004e741de0b Dave Chinner 2011-05-06 429 int count = 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 430
670ce93fef93bb Dave Chinner 2011-09-30 431 /*
43ff2122e6492b Christoph Hellwig 2012-04-23 432 * If we encountered pinned items or did not finish writing out all
0020a190cf3eac Dave Chinner 2021-08-10 433 * buffers the last time we ran, force a background CIL push to get the
0020a190cf3eac Dave Chinner 2021-08-10 434 * items unpinned in the near future. We do not wait on the CIL push as
0020a190cf3eac Dave Chinner 2021-08-10 435 * that could stall us for seconds if there is enough background IO
0020a190cf3eac Dave Chinner 2021-08-10 436 * load. Stalling for that long when the tail of the log is pinned and
0020a190cf3eac Dave Chinner 2021-08-10 437 * needs flushing will hard stop the transaction subsystem when log
0020a190cf3eac Dave Chinner 2021-08-10 438 * space runs out.
670ce93fef93bb Dave Chinner 2011-09-30 439 */
57e809561118a4 Matthew Wilcox 2018-03-07 440 if (ailp->ail_log_flush && ailp->ail_last_pushed_lsn == 0 &&
57e809561118a4 Matthew Wilcox 2018-03-07 441 (!list_empty_careful(&ailp->ail_buf_list) ||
43ff2122e6492b Christoph Hellwig 2012-04-23 442 xfs_ail_min_lsn(ailp))) {
57e809561118a4 Matthew Wilcox 2018-03-07 443 ailp->ail_log_flush = 0;
43ff2122e6492b Christoph Hellwig 2012-04-23 444
ff6d6af2351cae Bill O'Donnell 2015-10-12 445 XFS_STATS_INC(mp, xs_push_ail_flush);
0020a190cf3eac Dave Chinner 2021-08-10 446 xlog_cil_flush(mp->m_log);
670ce93fef93bb Dave Chinner 2011-09-30 447 }
670ce93fef93bb Dave Chinner 2011-09-30 448
57e809561118a4 Matthew Wilcox 2018-03-07 449 spin_lock(&ailp->ail_lock);
8375f922aaa6e7 Brian Foster 2012-06-28 450
29e90a4845ecee Dave Chinner 2022-03-17 451 /*
29e90a4845ecee Dave Chinner 2022-03-17 452 * If we have a sync push waiter, we always have to push till the AIL is
29e90a4845ecee Dave Chinner 2022-03-17 453 * empty. Update the target to point to the end of the AIL so that
29e90a4845ecee Dave Chinner 2022-03-17 454 * capture updates that occur after the sync push waiter has gone to
29e90a4845ecee Dave Chinner 2022-03-17 455 * sleep.
29e90a4845ecee Dave Chinner 2022-03-17 456 */
29e90a4845ecee Dave Chinner 2022-03-17 457 if (waitqueue_active(&ailp->ail_empty)) {
29e90a4845ecee Dave Chinner 2022-03-17 458 lip = xfs_ail_max(ailp);
29e90a4845ecee Dave Chinner 2022-03-17 459 if (lip)
29e90a4845ecee Dave Chinner 2022-03-17 460 target = lip->li_lsn;
29e90a4845ecee Dave Chinner 2022-03-17 461 } else {
57e809561118a4 Matthew Wilcox 2018-03-07 462 /* barrier matches the ail_target update in xfs_ail_push() */
8375f922aaa6e7 Brian Foster 2012-06-28 463 smp_rmb();
57e809561118a4 Matthew Wilcox 2018-03-07 464 target = ailp->ail_target;
57e809561118a4 Matthew Wilcox 2018-03-07 465 ailp->ail_target_prev = target;
29e90a4845ecee Dave Chinner 2022-03-17 466 }
8375f922aaa6e7 Brian Foster 2012-06-28 467
f376b45e861d8b Brian Foster 2020-07-16 468 /* we're done if the AIL is empty or our push has reached the end */
57e809561118a4 Matthew Wilcox 2018-03-07 469 lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->ail_last_pushed_lsn);
f376b45e861d8b Brian Foster 2020-07-16 470 if (!lip)
9e7004e741de0b Dave Chinner 2011-05-06 471 goto out_done;
^1da177e4c3f41 Linus Torvalds 2005-04-16 472
ff6d6af2351cae Bill O'Donnell 2015-10-12 473 XFS_STATS_INC(mp, xs_push_ail);
^1da177e4c3f41 Linus Torvalds 2005-04-16 474
249a8c1124653f David Chinner 2008-02-05 475 lsn = lip->li_lsn;
50e86686dfb287 Dave Chinner 2011-05-06 @476 while ((XFS_LSN_CMP(lip->li_lsn, target) <= 0)) {
249a8c1124653f David Chinner 2008-02-05 477 int lock_result;
43ff2122e6492b Christoph Hellwig 2012-04-23 478
^1da177e4c3f41 Linus Torvalds 2005-04-16 479 /*
904c17e6832845 Dave Chinner 2013-08-28 480 * Note that iop_push may unlock and reacquire the AIL lock. We
43ff2122e6492b Christoph Hellwig 2012-04-23 481 * rely on the AIL cursor implementation to be able to deal with
43ff2122e6492b Christoph Hellwig 2012-04-23 482 * the dropped lock.
^1da177e4c3f41 Linus Torvalds 2005-04-16 483 */
7f4d01f36a3ac1 Brian Foster 2017-08-08 484 lock_result = xfsaild_push_item(ailp, lip);
^1da177e4c3f41 Linus Torvalds 2005-04-16 485 switch (lock_result) {
^1da177e4c3f41 Linus Torvalds 2005-04-16 486 case XFS_ITEM_SUCCESS:
ff6d6af2351cae Bill O'Donnell 2015-10-12 487 XFS_STATS_INC(mp, xs_push_ail_success);
9e4c109ac82239 Christoph Hellwig 2011-10-11 488 trace_xfs_ail_push(lip);
9e4c109ac82239 Christoph Hellwig 2011-10-11 489
57e809561118a4 Matthew Wilcox 2018-03-07 490 ailp->ail_last_pushed_lsn = lsn;
^1da177e4c3f41 Linus Torvalds 2005-04-16 491 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 492
43ff2122e6492b Christoph Hellwig 2012-04-23 493 case XFS_ITEM_FLUSHING:
43ff2122e6492b Christoph Hellwig 2012-04-23 494 /*
cf085a1b5d2214 Joe Perches 2019-11-07 495 * The item or its backing buffer is already being
43ff2122e6492b Christoph Hellwig 2012-04-23 496 * flushed. The typical reason for that is that an
43ff2122e6492b Christoph Hellwig 2012-04-23 497 * inode buffer is locked because we already pushed the
43ff2122e6492b Christoph Hellwig 2012-04-23 498 * updates to it as part of inode clustering.
43ff2122e6492b Christoph Hellwig 2012-04-23 499 *
b63da6c8dfa9b2 Randy Dunlap 2020-08-05 500 * We do not want to stop flushing just because lots
cf085a1b5d2214 Joe Perches 2019-11-07 501 * of items are already being flushed, but we need to
43ff2122e6492b Christoph Hellwig 2012-04-23 502 * re-try the flushing relatively soon if most of the
cf085a1b5d2214 Joe Perches 2019-11-07 503 * AIL is being flushed.
43ff2122e6492b Christoph Hellwig 2012-04-23 504 */
ff6d6af2351cae Bill O'Donnell 2015-10-12 505 XFS_STATS_INC(mp, xs_push_ail_flushing);
43ff2122e6492b Christoph Hellwig 2012-04-23 506 trace_xfs_ail_flushing(lip);
17b38471c3c07a Christoph Hellwig 2011-10-11 507
43ff2122e6492b Christoph Hellwig 2012-04-23 508 flushing++;
57e809561118a4 Matthew Wilcox 2018-03-07 509 ailp->ail_last_pushed_lsn = lsn;
^1da177e4c3f41 Linus Torvalds 2005-04-16 510 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 511
^1da177e4c3f41 Linus Torvalds 2005-04-16 512 case XFS_ITEM_PINNED:
ff6d6af2351cae Bill O'Donnell 2015-10-12 513 XFS_STATS_INC(mp, xs_push_ail_pinned);
9e4c109ac82239 Christoph Hellwig 2011-10-11 514 trace_xfs_ail_pinned(lip);
9e4c109ac82239 Christoph Hellwig 2011-10-11 515
249a8c1124653f David Chinner 2008-02-05 516 stuck++;
57e809561118a4 Matthew Wilcox 2018-03-07 517 ailp->ail_log_flush++;
^1da177e4c3f41 Linus Torvalds 2005-04-16 518 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 519 case XFS_ITEM_LOCKED:
ff6d6af2351cae Bill O'Donnell 2015-10-12 520 XFS_STATS_INC(mp, xs_push_ail_locked);
9e4c109ac82239 Christoph Hellwig 2011-10-11 521 trace_xfs_ail_locked(lip);
43ff2122e6492b Christoph Hellwig 2012-04-23 522
249a8c1124653f David Chinner 2008-02-05 523 stuck++;
^1da177e4c3f41 Linus Torvalds 2005-04-16 524 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 525 default:
^1da177e4c3f41 Linus Torvalds 2005-04-16 526 ASSERT(0);
^1da177e4c3f41 Linus Torvalds 2005-04-16 527 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 528 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 529
249a8c1124653f David Chinner 2008-02-05 530 count++;
249a8c1124653f David Chinner 2008-02-05 531
^1da177e4c3f41 Linus Torvalds 2005-04-16 532 /*
249a8c1124653f David Chinner 2008-02-05 533 * Are there too many items we can't do anything with?
43ff2122e6492b Christoph Hellwig 2012-04-23 534 *
b63da6c8dfa9b2 Randy Dunlap 2020-08-05 535 * If we are skipping too many items because we can't flush
249a8c1124653f David Chinner 2008-02-05 536 * them or they are already being flushed, we back off and
249a8c1124653f David Chinner 2008-02-05 537 * given them time to complete whatever operation is being
249a8c1124653f David Chinner 2008-02-05 538 * done. i.e. remove pressure from the AIL while we can't make
249a8c1124653f David Chinner 2008-02-05 539 * progress so traversals don't slow down further inserts and
249a8c1124653f David Chinner 2008-02-05 540 * removals to/from the AIL.
249a8c1124653f David Chinner 2008-02-05 541 *
249a8c1124653f David Chinner 2008-02-05 542 * The value of 100 is an arbitrary magic number based on
249a8c1124653f David Chinner 2008-02-05 543 * observation.
^1da177e4c3f41 Linus Torvalds 2005-04-16 544 */
249a8c1124653f David Chinner 2008-02-05 545 if (stuck > 100)
249a8c1124653f David Chinner 2008-02-05 546 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 547
af3e40228fb2db Dave Chinner 2011-07-18 548 lip = xfs_trans_ail_cursor_next(ailp, &cur);
249a8c1124653f David Chinner 2008-02-05 549 if (lip == NULL)
249a8c1124653f David Chinner 2008-02-05 550 break;
249a8c1124653f David Chinner 2008-02-05 551 lsn = lip->li_lsn;
^1da177e4c3f41 Linus Torvalds 2005-04-16 552 }
f376b45e861d8b Brian Foster 2020-07-16 553
f376b45e861d8b Brian Foster 2020-07-16 554 out_done:
e4a1e29cb0ace3 Eric Sandeen 2014-04-14 555 xfs_trans_ail_cursor_done(&cur);
57e809561118a4 Matthew Wilcox 2018-03-07 556 spin_unlock(&ailp->ail_lock);
^1da177e4c3f41 Linus Torvalds 2005-04-16 557
57e809561118a4 Matthew Wilcox 2018-03-07 558 if (xfs_buf_delwri_submit_nowait(&ailp->ail_buf_list))
57e809561118a4 Matthew Wilcox 2018-03-07 559 ailp->ail_log_flush++;
d808f617ad00a4 Dave Chinner 2010-02-02 560
43ff2122e6492b Christoph Hellwig 2012-04-23 561 if (!count || XFS_LSN_CMP(lsn, target) >= 0) {
249a8c1124653f David Chinner 2008-02-05 562 /*
43ff2122e6492b Christoph Hellwig 2012-04-23 563 * We reached the target or the AIL is empty, so wait a bit
43ff2122e6492b Christoph Hellwig 2012-04-23 564 * longer for I/O to complete and remove pushed items from the
43ff2122e6492b Christoph Hellwig 2012-04-23 565 * AIL before we start the next scan from the start of the AIL.
249a8c1124653f David Chinner 2008-02-05 566 */
453eac8a9aa417 Dave Chinner 2010-01-11 567 tout = 50;
57e809561118a4 Matthew Wilcox 2018-03-07 568 ailp->ail_last_pushed_lsn = 0;
43ff2122e6492b Christoph Hellwig 2012-04-23 569 } else if (((stuck + flushing) * 100) / count > 90) {
249a8c1124653f David Chinner 2008-02-05 570 /*
43ff2122e6492b Christoph Hellwig 2012-04-23 571 * Either there is a lot of contention on the AIL or we are
43ff2122e6492b Christoph Hellwig 2012-04-23 572 * stuck due to operations in progress. "Stuck" in this case
43ff2122e6492b Christoph Hellwig 2012-04-23 573 * is defined as >90% of the items we tried to push were stuck.
249a8c1124653f David Chinner 2008-02-05 574 *
249a8c1124653f David Chinner 2008-02-05 575 * Backoff a bit more to allow some I/O to complete before
43ff2122e6492b Christoph Hellwig 2012-04-23 576 * restarting from the start of the AIL. This prevents us from
43ff2122e6492b Christoph Hellwig 2012-04-23 577 * spinning on the same items, and if they are pinned will all
43ff2122e6492b Christoph Hellwig 2012-04-23 578 * the restart to issue a log force to unpin the stuck items.
249a8c1124653f David Chinner 2008-02-05 579 */
453eac8a9aa417 Dave Chinner 2010-01-11 580 tout = 20;
57e809561118a4 Matthew Wilcox 2018-03-07 581 ailp->ail_last_pushed_lsn = 0;
43ff2122e6492b Christoph Hellwig 2012-04-23 582 } else {
43ff2122e6492b Christoph Hellwig 2012-04-23 583 /*
43ff2122e6492b Christoph Hellwig 2012-04-23 584 * Assume we have more work to do in a short while.
43ff2122e6492b Christoph Hellwig 2012-04-23 585 */
43ff2122e6492b Christoph Hellwig 2012-04-23 586 tout = 10;
^1da177e4c3f41 Linus Torvalds 2005-04-16 587 }
0bf6a5bd4b55b4 Dave Chinner 2011-04-08 588
0030807c66f058 Christoph Hellwig 2011-10-11 589 return tout;
0030807c66f058 Christoph Hellwig 2011-10-11 590 }
0030807c66f058 Christoph Hellwig 2011-10-11 591
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
2022-03-17 5:39 [PATCH 0/7 v4] xfs: log recovery fixes Dave Chinner
@ 2022-03-17 5:39 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2022-03-17 5:39 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
xfs_ail_push_all_sync() has a loop like this:
while max_ail_lsn {
prepare_to_wait(ail_empty)
target = max_ail_lsn
wake_up(ail_task);
schedule()
}
Which is designed to sleep until the AIL is emptied. When
xfs_ail_update_finish() moves the tail of the log, it does:
if (list_empty(&ailp->ail_head))
wake_up_all(&ailp->ail_empty);
So it will only wake up the sync push waiter when the AIL goes
empty. If, by the time the push waiter has woken, the AIL has more
in it, it will reset the target, wake the push task and go back to
sleep.
The problem here is that if the AIL is having items added to it
when xfs_ail_push_all_sync() is called, then they may get inserted
into the AIL at a LSN higher than the target LSN. At this point,
xfsaild_push() will see that the target is X, the item LSNs are
(X+N) and skip over them, hence never pushing the out.
The result of this the AIL will not get emptied by the AIL push
thread, hence xfs_ail_finish_update() will never see the AIL being
empty even if it moves the tail. Hence xfs_ail_push_all_sync() never
gets woken and hence cannot update the push target to capture the
items beyond the current target on the LSN.
This is a TOCTOU type of issue so the way to avoid it is to not
use the push target at all for sync pushes. We know that a sync push
is being requested by the fact the ail_empty wait queue is active,
hence the xfsaild can just set the target to max_ail_lsn on every
push that we see the wait queue active. Hence we no longer will
leave items on the AIL that are beyond the LSN sampled at the start
of a sync push.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_trans_ail.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 2a8c8dc54c95..1b52952097c1 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -448,10 +448,22 @@ xfsaild_push(
spin_lock(&ailp->ail_lock);
- /* barrier matches the ail_target update in xfs_ail_push() */
- smp_rmb();
- target = ailp->ail_target;
- ailp->ail_target_prev = target;
+ /*
+ * If we have a sync push waiter, we always have to push till the AIL is
+ * empty. Update the target to point to the end of the AIL so that
+ * capture updates that occur after the sync push waiter has gone to
+ * sleep.
+ */
+ if (waitqueue_active(&ailp->ail_empty)) {
+ lip = xfs_ail_max(ailp);
+ if (lip)
+ target = lip->li_lsn;
+ } else {
+ /* barrier matches the ail_target update in xfs_ail_push() */
+ smp_rmb();
+ target = ailp->ail_target;
+ ailp->ail_target_prev = target;
+ }
/* we're done if the AIL is empty or our push has reached the end */
lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->ail_last_pushed_lsn);
@@ -724,7 +736,6 @@ xfs_ail_push_all_sync(
spin_lock(&ailp->ail_lock);
while ((lip = xfs_ail_max(ailp)) != NULL) {
prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
- ailp->ail_target = lip->li_lsn;
wake_up_process(ailp->ail_task);
spin_unlock(&ailp->ail_lock);
schedule();
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
2022-03-15 19:17 ` Darrick J. Wong
@ 2022-03-15 21:29 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2022-03-15 21:29 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Mar 15, 2022 at 12:17:35PM -0700, Darrick J. Wong wrote:
> On Tue, Mar 15, 2022 at 05:42:37PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > xfs_ail_push_all_sync() has a loop like this:
> >
> > while max_ail_lsn {
> > prepare_to_wait(ail_empty)
> > target = max_ail_lsn
> > wake_up(ail_task);
> > schedule()
> > }
> >
> > Which is designed to sleep until the AIL is emptied. When
> > xfs_ail_finish_update() moves the tail of the log, it does:
> >
> > if (list_empty(&ailp->ail_head))
> > wake_up_all(&ailp->ail_empty);
> >
> > So it will only wake up the sync push waiter when the AIL goes
> > empty. If, by the time the push waiter has woken, the AIL has more
> > in it, it will reset the target, wake the push task and go back to
> > sleep.
> >
> > The problem here is that if the AIL is having items added to it
> > when xfs_ail_push_all_sync() is called, then they may get inserted
> > into the AIL at a LSN higher than the target LSN. At this point,
> > xfsaild_push() will see that the target is X, the item LSNs are
> > (X+N) and skip over them, hence never pushing the out.
> >
> > The result of this the AIL will not get emptied by the AIL push
> > thread, hence xfs_ail_finish_update() will never see the AIL being
> > empty even if it moves the tail. Hence xfs_ail_push_all_sync() never
> > gets woken and hence cannot update the push target to capture the
> > items beyond the current target on the LSN.
> >
> > This is a TOCTOU type of issue so the way to avoid it is to not
> > use the push target at all for sync pushes. We know that a sync push
> > is being requested by the fact the ail_empty wait queue is active,
> > hence the xfsaild can just set the target to max_ail_lsn on every
> > push that we see the wait queue active. Hence we no longer will
> > leave items on the AIL that are beyond the LSN sampled at the start
> > of a sync push.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > fs/xfs/xfs_trans_ail.c | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 2a8c8dc54c95..1b52952097c1 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -448,10 +448,22 @@ xfsaild_push(
> >
> > spin_lock(&ailp->ail_lock);
> >
> > - /* barrier matches the ail_target update in xfs_ail_push() */
> > - smp_rmb();
> > - target = ailp->ail_target;
> > - ailp->ail_target_prev = target;
> > + /*
> > + * If we have a sync push waiter, we always have to push till the AIL is
> > + * empty. Update the target to point to the end of the AIL so that
> > + * capture updates that occur after the sync push waiter has gone to
> > + * sleep.
> > + */
> > + if (waitqueue_active(&ailp->ail_empty)) {
> > + lip = xfs_ail_max(ailp);
> > + if (lip)
> > + target = lip->li_lsn;
> > + } else {
> > + /* barrier matches the ail_target update in xfs_ail_push() */
> > + smp_rmb();
>
> Doesn't the spin_lock provide the required rmb? I think it's
> unnecessary given that, but I also don't think it hurts anything, so:
No. xfs_ail_push() does not take the ail_lock to update
ail->ail_target on 64 bit systems(*). Spin locks only provide memory
barriers between critical sections within the lock/unlock calls, and
even then the barrier is in the unlock -> lock direction only. i.e.
what is written before the unlock in one critical section is
guaranteed to be read after the lock that starts the next critical
section.
Instead, xfs_ail_push() has smp_wmb() calls around setting the
target to ensure that all ail state updates done -before the wmb- are
seen by reads done -after the rmb- above. These memory barriers
could probably be replaced with a smp_store_release() and
smp_load_acquire() pair, because that is effectively what they are
implementing but the implementation predates those primitives.
OTOH, we don't need a rmb before the new waitqueue_active check
because all the waitqueue manipulations are done under the ail_lock.
Hence the ail_lock provides the memory barriers for that branch.
IOWs, the smp_rmb() is still necessary for the lockless
xfs_ail_push() update path, just like it was before this patch.
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Thanks!
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
2022-03-15 6:42 ` [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates Dave Chinner
2022-03-15 15:14 ` Chandan Babu R
@ 2022-03-15 19:17 ` Darrick J. Wong
2022-03-15 21:29 ` Dave Chinner
1 sibling, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2022-03-15 19:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, Mar 15, 2022 at 05:42:37PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> xfs_ail_push_all_sync() has a loop like this:
>
> while max_ail_lsn {
> prepare_to_wait(ail_empty)
> target = max_ail_lsn
> wake_up(ail_task);
> schedule()
> }
>
> Which is designed to sleep until the AIL is emptied. When
> xfs_ail_finish_update() moves the tail of the log, it does:
>
> if (list_empty(&ailp->ail_head))
> wake_up_all(&ailp->ail_empty);
>
> So it will only wake up the sync push waiter when the AIL goes
> empty. If, by the time the push waiter has woken, the AIL has more
> in it, it will reset the target, wake the push task and go back to
> sleep.
>
> The problem here is that if the AIL is having items added to it
> when xfs_ail_push_all_sync() is called, then they may get inserted
> into the AIL at a LSN higher than the target LSN. At this point,
> xfsaild_push() will see that the target is X, the item LSNs are
> (X+N) and skip over them, hence never pushing the out.
>
> The result of this the AIL will not get emptied by the AIL push
> thread, hence xfs_ail_finish_update() will never see the AIL being
> empty even if it moves the tail. Hence xfs_ail_push_all_sync() never
> gets woken and hence cannot update the push target to capture the
> items beyond the current target on the LSN.
>
> This is a TOCTOU type of issue so the way to avoid it is to not
> use the push target at all for sync pushes. We know that a sync push
> is being requested by the fact the ail_empty wait queue is active,
> hence the xfsaild can just set the target to max_ail_lsn on every
> push that we see the wait queue active. Hence we no longer will
> leave items on the AIL that are beyond the LSN sampled at the start
> of a sync push.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_trans_ail.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 2a8c8dc54c95..1b52952097c1 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -448,10 +448,22 @@ xfsaild_push(
>
> spin_lock(&ailp->ail_lock);
>
> - /* barrier matches the ail_target update in xfs_ail_push() */
> - smp_rmb();
> - target = ailp->ail_target;
> - ailp->ail_target_prev = target;
> + /*
> + * If we have a sync push waiter, we always have to push till the AIL is
> + * empty. Update the target to point to the end of the AIL so that
> + * capture updates that occur after the sync push waiter has gone to
> + * sleep.
> + */
> + if (waitqueue_active(&ailp->ail_empty)) {
> + lip = xfs_ail_max(ailp);
> + if (lip)
> + target = lip->li_lsn;
> + } else {
> + /* barrier matches the ail_target update in xfs_ail_push() */
> + smp_rmb();
Doesn't the spin_lock provide the required rmb? I think it's
unnecessary given that, but I also don't think it hurts anything, so:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> + target = ailp->ail_target;
> + ailp->ail_target_prev = target;
> + }
>
> /* we're done if the AIL is empty or our push has reached the end */
> lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->ail_last_pushed_lsn);
> @@ -724,7 +736,6 @@ xfs_ail_push_all_sync(
> spin_lock(&ailp->ail_lock);
> while ((lip = xfs_ail_max(ailp)) != NULL) {
> prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
> - ailp->ail_target = lip->li_lsn;
> wake_up_process(ailp->ail_task);
> spin_unlock(&ailp->ail_lock);
> schedule();
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
2022-03-15 6:42 ` [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates Dave Chinner
@ 2022-03-15 15:14 ` Chandan Babu R
2022-03-15 19:17 ` Darrick J. Wong
1 sibling, 0 replies; 8+ messages in thread
From: Chandan Babu R @ 2022-03-15 15:14 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On 15 Mar 2022 at 12:12, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> xfs_ail_push_all_sync() has a loop like this:
>
> while max_ail_lsn {
> prepare_to_wait(ail_empty)
> target = max_ail_lsn
> wake_up(ail_task);
> schedule()
> }
>
> Which is designed to sleep until the AIL is emptied. When
> xfs_ail_finish_update() moves the tail of the log, it does:
... xfs_ail_update_finish() ...
>
> if (list_empty(&ailp->ail_head))
> wake_up_all(&ailp->ail_empty);
>
> So it will only wake up the sync push waiter when the AIL goes
> empty. If, by the time the push waiter has woken, the AIL has more
> in it, it will reset the target, wake the push task and go back to
> sleep.
>
> The problem here is that if the AIL is having items added to it
> when xfs_ail_push_all_sync() is called, then they may get inserted
> into the AIL at a LSN higher than the target LSN. At this point,
> xfsaild_push() will see that the target is X, the item LSNs are
> (X+N) and skip over them, hence never pushing the out.
>
> The result of this the AIL will not get emptied by the AIL push
> thread, hence xfs_ail_finish_update() will never see the AIL being
> empty even if it moves the tail. Hence xfs_ail_push_all_sync() never
> gets woken and hence cannot update the push target to capture the
> items beyond the current target on the LSN.
>
> This is a TOCTOU type of issue so the way to avoid it is to not
> use the push target at all for sync pushes. We know that a sync push
> is being requested by the fact the ail_empty wait queue is active,
> hence the xfsaild can just set the target to max_ail_lsn on every
> push that we see the wait queue active. Hence we no longer will
> leave items on the AIL that are beyond the LSN sampled at the start
> of a sync push.
>
The fix seems to logically correct.
Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_trans_ail.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 2a8c8dc54c95..1b52952097c1 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -448,10 +448,22 @@ xfsaild_push(
>
> spin_lock(&ailp->ail_lock);
>
> - /* barrier matches the ail_target update in xfs_ail_push() */
> - smp_rmb();
> - target = ailp->ail_target;
> - ailp->ail_target_prev = target;
> + /*
> + * If we have a sync push waiter, we always have to push till the AIL is
> + * empty. Update the target to point to the end of the AIL so that
> + * capture updates that occur after the sync push waiter has gone to
> + * sleep.
> + */
> + if (waitqueue_active(&ailp->ail_empty)) {
> + lip = xfs_ail_max(ailp);
> + if (lip)
> + target = lip->li_lsn;
> + } else {
> + /* barrier matches the ail_target update in xfs_ail_push() */
> + smp_rmb();
> + target = ailp->ail_target;
> + ailp->ail_target_prev = target;
> + }
>
> /* we're done if the AIL is empty or our push has reached the end */
> lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->ail_last_pushed_lsn);
> @@ -724,7 +736,6 @@ xfs_ail_push_all_sync(
> spin_lock(&ailp->ail_lock);
> while ((lip = xfs_ail_max(ailp)) != NULL) {
> prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
> - ailp->ail_target = lip->li_lsn;
> wake_up_process(ailp->ail_task);
> spin_unlock(&ailp->ail_lock);
> schedule();
--
chandan
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
2022-03-15 6:42 [PATCH 0/7 v3] xfs: log recovery fixes Dave Chinner
@ 2022-03-15 6:42 ` Dave Chinner
2022-03-15 15:14 ` Chandan Babu R
2022-03-15 19:17 ` Darrick J. Wong
0 siblings, 2 replies; 8+ messages in thread
From: Dave Chinner @ 2022-03-15 6:42 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
xfs_ail_push_all_sync() has a loop like this:
while max_ail_lsn {
prepare_to_wait(ail_empty)
target = max_ail_lsn
wake_up(ail_task);
schedule()
}
Which is designed to sleep until the AIL is emptied. When
xfs_ail_finish_update() moves the tail of the log, it does:
if (list_empty(&ailp->ail_head))
wake_up_all(&ailp->ail_empty);
So it will only wake up the sync push waiter when the AIL goes
empty. If, by the time the push waiter has woken, the AIL has more
in it, it will reset the target, wake the push task and go back to
sleep.
The problem here is that if the AIL is having items added to it
when xfs_ail_push_all_sync() is called, then they may get inserted
into the AIL at a LSN higher than the target LSN. At this point,
xfsaild_push() will see that the target is X, the item LSNs are
(X+N) and skip over them, hence never pushing the out.
The result of this the AIL will not get emptied by the AIL push
thread, hence xfs_ail_finish_update() will never see the AIL being
empty even if it moves the tail. Hence xfs_ail_push_all_sync() never
gets woken and hence cannot update the push target to capture the
items beyond the current target on the LSN.
This is a TOCTOU type of issue so the way to avoid it is to not
use the push target at all for sync pushes. We know that a sync push
is being requested by the fact the ail_empty wait queue is active,
hence the xfsaild can just set the target to max_ail_lsn on every
push that we see the wait queue active. Hence we no longer will
leave items on the AIL that are beyond the LSN sampled at the start
of a sync push.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_trans_ail.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 2a8c8dc54c95..1b52952097c1 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -448,10 +448,22 @@ xfsaild_push(
spin_lock(&ailp->ail_lock);
- /* barrier matches the ail_target update in xfs_ail_push() */
- smp_rmb();
- target = ailp->ail_target;
- ailp->ail_target_prev = target;
+ /*
+ * If we have a sync push waiter, we always have to push till the AIL is
+ * empty. Update the target to point to the end of the AIL so that
+ * capture updates that occur after the sync push waiter has gone to
+ * sleep.
+ */
+ if (waitqueue_active(&ailp->ail_empty)) {
+ lip = xfs_ail_max(ailp);
+ if (lip)
+ target = lip->li_lsn;
+ } else {
+ /* barrier matches the ail_target update in xfs_ail_push() */
+ smp_rmb();
+ target = ailp->ail_target;
+ ailp->ail_target_prev = target;
+ }
/* we're done if the AIL is empty or our push has reached the end */
lip = xfs_trans_ail_cursor_first(ailp, &cur, ailp->ail_last_pushed_lsn);
@@ -724,7 +736,6 @@ xfs_ail_push_all_sync(
spin_lock(&ailp->ail_lock);
while ((lip = xfs_ail_max(ailp)) != NULL) {
prepare_to_wait(&ailp->ail_empty, &wait, TASK_UNINTERRUPTIBLE);
- ailp->ail_target = lip->li_lsn;
wake_up_process(ailp->ail_task);
spin_unlock(&ailp->ail_lock);
schedule();
--
2.35.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-20 15:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-18 10:08 [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates kernel test robot
2022-03-20 15:17 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2022-03-17 14:30 kernel test robot
2022-03-17 5:39 [PATCH 0/7 v4] xfs: log recovery fixes Dave Chinner
2022-03-17 5:39 ` [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates Dave Chinner
2022-03-15 6:42 [PATCH 0/7 v3] xfs: log recovery fixes Dave Chinner
2022-03-15 6:42 ` [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates Dave Chinner
2022-03-15 15:14 ` Chandan Babu R
2022-03-15 19:17 ` Darrick J. Wong
2022-03-15 21:29 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.