Re: [PATCH 07/17] btrfs: push extent lock into run_delalloc_nocow

[David Sterba <dsterba@suse.cz>]

On Tue, Apr 23, 2024 at 12:49:14PM -0400, Josef Bacik wrote:
> On Tue, Apr 23, 2024 at 06:33:25AM -0500, Goldwyn Rodrigues wrote:
> > On 10:35 17/04, Josef Bacik wrote:
> > > run_delalloc_nocow is a bit special as it walks through the file extents
> > > for the inode and determines what it can nocow and what it can't.  This
> > > is the more complicated area for extent locking, so start with this
> > > function.
> > > 
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > 
> > 
> > > ---
> > >  fs/btrfs/inode.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index 2083005f2828..f14b3cecce47 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -1977,6 +1977,8 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,
> > >  	 */
> > >  	ASSERT(!btrfs_is_zoned(fs_info) || btrfs_is_data_reloc_root(root));
> > >  
> > > +	lock_extent(&inode->io_tree, start, end, NULL);
> > > +
> > >  	path = btrfs_alloc_path();
> > >  	if (!path) {
> > >  		ret = -ENOMEM;
> > > @@ -2249,11 +2251,6 @@ int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page
> > >  	const bool zoned = btrfs_is_zoned(inode->root->fs_info);
> > >  	int ret;
> > >  
> > > -	/*
> > > -	 * We're unlocked by the different fill functions below.
> > > -	 */
> > > -	lock_extent(&inode->io_tree, start, end, NULL);
> > > -
> > 
> > So, you are adding this hunk in the previous patch and (re)moving it here.
> > Do you think it would be better to merge this with the previous patch?
> 
> It depends?  I did it like this so people could follow closely as I pushed the
> locking down.  Most of these "push extent lock into.." patches do a variation of
> this, where I push it down into a function and move the lock down in the set of
> things.  I'm happy to merge them together, but I split it this way so my logic
> could be followed.

The incremental changes are better for review, moving locks affects code
before and after and it's more convienient to review each step as the
amount of information and context is bearable. So I'm fine with the way
you did it.