lkml.org 
[lkml]   [2020]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] xfs: Fix false positive lockdep warning with sb_internal & fs_reclaim
From
Date
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

\
 
 \ /
  Last update: 2020-06-05 02:47    [W:0.063 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site