Messages in this thread | | | Subject | Re: [PATCH v2] xfs: Fix false positive lockdep warning with sb_internal & fs_reclaim | From | Waiman Long <> | Date | Thu, 4 Jun 2020 20:46:23 -0400 |
| |
On 6/4/20 7:13 PM, Dave Chinner wrote: > On Thu, Jun 04, 2020 at 05:01:30PM -0400, Waiman Long wrote: >> --- >> fs/xfs/xfs_log.c | 3 ++- >> fs/xfs/xfs_trans.c | 8 +++++++- >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c >> index 00fda2e8e738..d273d4e74ef8 100644 >> --- a/fs/xfs/xfs_log.c >> +++ b/fs/xfs/xfs_log.c >> @@ -433,7 +433,8 @@ xfs_log_reserve( >> XFS_STATS_INC(mp, xs_try_logspace); >> >> ASSERT(*ticp == NULL); >> - tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, 0); >> + tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, >> + mp->m_super->s_writers.frozen ? KM_NOLOCKDEP : 0); >> *ticp = tic; > Hi Waiman, > > As I originally stated when you posted this the first time 6 months > ago: we are not going to spread this sort of conditional gunk though > the XFS codebase just to shut up lockdep false positives. > > I pointed you at the way to conditionally turn of lockdep for > operations where we are doing transactions when the filesystem has > already frozen the transaction subsystem. That is: > >> >> xlog_grant_push_ail(log, tic->t_cnt ? tic->t_unit_res * tic->t_cnt >> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c >> index 3c94e5ff4316..3a9f394a0f02 100644 >> --- a/fs/xfs/xfs_trans.c >> +++ b/fs/xfs/xfs_trans.c >> @@ -261,8 +261,14 @@ xfs_trans_alloc( >> * Allocate the handle before we do our freeze accounting and setting up >> * GFP_NOFS allocation context so that we avoid lockdep false positives >> * by doing GFP_KERNEL allocations inside sb_start_intwrite(). >> + * >> + * To prevent false positive lockdep warning of circular locking >> + * dependency between sb_internal and fs_reclaim, disable the >> + * acquisition of the fs_reclaim pseudo-lock when the superblock >> + * has been frozen or in the process of being frozen. >> */ >> - tp = kmem_zone_zalloc(xfs_trans_zone, 0); >> + tp = kmem_zone_zalloc(xfs_trans_zone, >> + mp->m_super->s_writers.frozen ? KM_NOLOCKDEP : 0); >> if (!(flags & XFS_TRANS_NO_WRITECOUNT)) > We only should be setting KM_NOLOCKDEP when XFS_TRANS_NO_WRITECOUNT > is set. That's the flag that transactions set when they run in a > fully frozen context to avoid deadlocking with the freeze in > progress, and that's the only case where we should be turning off > lockdep. > > And, as I also mentioned, this should be done via a process flag - > PF_MEMALLOC_NOLOCKDEP - so that it is automatically inherited by > all subsequent memory allocations done in this path. That way we > only need this wrapping code in xfs_trans_alloc(): > > if (flags & XFS_TRANS_NO_WRITECOUNT) > memalloc_nolockdep_save() > > ..... > > if (flags & XFS_TRANS_NO_WRITECOUNT) > memalloc_nolockdep_restore() > > and nothing else needs to change. > > Cheers, > > Dave.
Thanks for the reminder, I will look into that.
Cheers, Longman
| |