xfs
[Top] [All Lists]

Re: [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: use GFP_NOFS argument in radix_tree_preload
From: Taesoo Kim <taesoo@xxxxxxxxxx>
Date: Mon, 23 Mar 2015 01:37:15 -0400
Cc: Sanidhya Kashyap <sanidhya.gatech@xxxxxxxxx>, xfs@xxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, changwoo@xxxxxxxxxx, sanidhya@xxxxxxxxxx, blee@xxxxxxxxxx, csong84@xxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=J0lxcYFRRB5lejhcuKEfe+oY51XMhelZBFhSy+lUxGk=; b=VSRfiRw2/6IaHNFU/d4CxBo5WG9t8pd/dszmokYkhvhksvXf45TVUiv/vInuE5iL8H mgKEnUxIys5ek1Baf9qZiszSiUNiKZOzzBLcMKOTs+VG3Rcde31X/OTGRIIiAGH66HN1 IQ9lYA8aLbXz5rXosI6SmFxx/O2B8DR1I+B5/HNfCVmuwAYJBjhxB5By3KXEE0xKL5H+ 5mo086yirYcWpxWwKtp7YHvDBWXBRdv5moEwEZP0BWHLTW2ruT9zzH8l4qowoS/QFYT+ Fg3SYzht8zDp8PVZQrWQr2bEIRfqongG5IwyA1EURQIvMneT8w782FqJ4tZUodWH6EXK SqtA==
In-reply-to: <20150323052449.GO28621@dastard>
References: <1427087183-20391-1-git-send-email-sanidhya.gatech@xxxxxxxxx> <20150323052449.GO28621@dastard>
Sender: Taesoo Kim <tsgatesv@xxxxxxxxx>
User-agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Hi Dave,

Thank you for letting us know. Since we are not an expert of XFS (nor
want to be), we really want to let you guys know it's potential bug
that you might miss (we are helping you!). And that's why Sanidhya
asked (rather than sending a patch) at the first place.

I agree that the comment is misleading and not correct, but probably
encouraging a student who spend times to clean up your mistake
might be better way to influence him rather than shouting :)

Taesoo

On 03/23/15 at 04:24pm, Dave Chinner wrote:
> On Mon, Mar 23, 2015 at 01:06:23AM -0400, Sanidhya Kashyap wrote:
> > From: Byoungyoung Lee <blee@xxxxxxxxxx>
> > 
> > Following the convention of other file systems, GFP_NOFS
> > should be used as an argument for radix_tree_preload() instead
> > of GFP_KERNEL.
> 
> "convention of other filesystems" is not a reason for changing from
> GFP_KERNEL to GFP_NOFS. There are rules for when GFP_NOFS needs to
> be used, and so we only need to change the code if one of those
> rules are triggered. i.e. inside a transaction, holding a lock that
> memory reclaim might require to make progress (e.g. ip->i_ilock,
> buffer locks, etc). The context in which the allocation is made will
> tell you whether GFP_KERNEL is safe or not.
> 
> So while the change probably needs to be made, it needs to be made
> for the right reasons. I haven't looked at the code, but I have
> a pretty good idea of the context the allocation is being made
> under. I'd suggest documenting the call path down to
> xfs_mru_cache_insert(), because that will tell you exactly what
> context the allocaiton is being made in and hence tell everyone else
> the real reason we need to make this change...
> 
> Call me picky, pendantic and/or annoying, but if you are looking at
> validating/correcting allocation flags then you need to understand
> the rules and context in which the allocation is being made...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>