From: Dave Chinner <david@fromorbit.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: kbuild@lists.01.org, linux-xfs@vger.kernel.org, lkp@intel.com,
kbuild-all@lists.01.org
Subject: Re: [kbuild] Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
Date: Sat, 19 Mar 2022 07:59:12 +1100 [thread overview]
Message-ID: <20220318205912.GF1544202@dread.disaster.area> (raw)
In-Reply-To: <202203172212.pRLbx3jA-lkp@intel.com>
On Fri, Mar 18, 2022 at 11:10:22AM +0300, Dan Carpenter wrote:
> Hi Dave,
>
> 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: parisc-randconfig-m031-20220317 (https://download.01.org/0day-ci/archive/20220317/202203172212.pRLbx3jA-lkp@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
>
> 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;
>
> No else path.
Target will only be uninitialised here if the AIL is empty.
> 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);
>
> "lip" re-assigned here
If the AIL is empty, this will return NULL. Hence if xfs_ail_max()
returns NULL, so will this. Hence:
>
> f376b45e861d8b Brian Foster 2020-07-16 470 if (!lip)
> 9e7004e741de0b Dave Chinner 2011-05-06 471 goto out_done;
We take this path, and never reference target...
> ^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)) {
> ^^^^^^
And this path will only be taken if there are items in the AIL,
and in that case we are guaranteed to have initialised target....
Not a bug.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com>
To: kbuild-all@lists.01.org
Subject: Re: [kbuild] Re: [PATCH 3/7] xfs: xfs_ail_push_all_sync() stalls when racing with updates
Date: Sat, 19 Mar 2022 07:59:12 +1100 [thread overview]
Message-ID: <20220318205912.GF1544202@dread.disaster.area> (raw)
In-Reply-To: <202203172212.pRLbx3jA-lkp@intel.com>
[-- Attachment #1: Type: text/plain, Size: 6296 bytes --]
On Fri, Mar 18, 2022 at 11:10:22AM +0300, Dan Carpenter wrote:
> Hi Dave,
>
> 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: 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
>
> 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;
>
> No else path.
Target will only be uninitialised here if the AIL is empty.
> 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);
>
> "lip" re-assigned here
If the AIL is empty, this will return NULL. Hence if xfs_ail_max()
returns NULL, so will this. Hence:
>
> f376b45e861d8b Brian Foster 2020-07-16 470 if (!lip)
> 9e7004e741de0b Dave Chinner 2011-05-06 471 goto out_done;
We take this path, and never reference target...
> ^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)) {
> ^^^^^^
And this path will only be taken if there are items in the AIL,
and in that case we are guaranteed to have initialised target....
Not a bug.
Cheers,
Dave.
--
Dave Chinner
david(a)fromorbit.com
next prev parent reply other threads:[~2022-03-18 20:59 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-17 5:39 [PATCH 0/7 v4] xfs: log recovery fixes Dave Chinner
2022-03-17 5:39 ` [PATCH 1/7] xfs: log worker needs to start before intent/unlink recovery Dave Chinner
2022-03-17 5:39 ` [PATCH 2/7] xfs: check buffer pin state after locking in delwri_submit 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-17 14:30 ` kernel test robot
2022-03-18 8:10 ` [kbuild] " Dan Carpenter
2022-03-18 8:10 ` Dan Carpenter
2022-03-18 20:59 ` Dave Chinner [this message]
2022-03-18 20:59 ` Dave Chinner
2022-03-17 5:39 ` [PATCH 4/7] xfs: async CIL flushes need pending pushes to be made stable Dave Chinner
2022-03-17 6:51 ` Chandan Babu R
2022-03-17 5:39 ` [PATCH 5/7] xfs: log items should have a xlog pointer, not a mount Dave Chinner
2022-03-17 5:39 ` [PATCH 6/7] xfs: AIL should be log centric Dave Chinner
2022-03-17 5:39 ` [PATCH 7/7] xfs: xfs_is_shutdown vs xlog_is_shutdown cage fight Dave Chinner
2022-03-17 16:11 ` Darrick J. Wong
2022-03-18 11:30 ` Chandan Babu R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220318205912.GF1544202@dread.disaster.area \
--to=david@fromorbit.com \
--cc=dan.carpenter@oracle.com \
--cc=kbuild-all@lists.01.org \
--cc=kbuild@lists.01.org \
--cc=linux-xfs@vger.kernel.org \
--cc=lkp@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.