Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 21:47:06 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Ted Tso <tytso@mit.edu>, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 06/10] ext2: Factor our freeing of xattr block reference
Message-ID: <20220714161706.7itdgwl5cjbgdwzs@riteshh-domain>
References: <20220712104519.29887-1-jack@suse.cz>
 <20220712105436.32204-6-jack@suse.cz>
 <20220714123714.xxqo7nnde6xacriu@riteshh-domain>
 <20220714145538.jbrbobhi5ppvuxka@quack3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220714145538.jbrbobhi5ppvuxka@quack3>
Precedence: bulk

On 22/07/14 04:55PM, Jan Kara wrote:
> On Thu 14-07-22 18:07:14, Ritesh Harjani wrote:
> > On 22/07/12 12:54PM, Jan Kara wrote:
> > > Free of xattr block reference is opencode in two places. Factor it out
> > > into a separate function and use it.
> >
> > Looked into the refactoring logic. The patch looks good to me.
> > Small queries below -
> >
> > >
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/ext2/xattr.c | 90 +++++++++++++++++++++----------------------------
> > >  1 file changed, 38 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> > > index 841fa6d9d744..9885294993ef 100644
> > > --- a/fs/ext2/xattr.c
> > > +++ b/fs/ext2/xattr.c
> > > @@ -651,6 +651,42 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
> > >  	return error;
> > >  }
> > >
> > > +static void ext2_xattr_release_block(struct inode *inode,
> > > +				     struct buffer_head *bh)
> > > +{
> > > +	struct mb_cache *ea_block_cache = EA_BLOCK_CACHE(inode);
> > > +
> > > +	lock_buffer(bh);
> > > +	if (HDR(bh)->h_refcount == cpu_to_le32(1)) {
> > > +		__u32 hash = le32_to_cpu(HDR(bh)->h_hash);
> > > +
> > > +		/*
> > > +		 * This must happen under buffer lock for
> > > +		 * ext2_xattr_set2() to reliably detect freed block
> > > +		 */
> > > +		mb_cache_entry_delete(ea_block_cache, hash,
> > > +				      bh->b_blocknr);
> > > +		/* Free the old block. */
> > > +		ea_bdebug(bh, "freeing");
> > > +		ext2_free_blocks(inode, bh->b_blocknr, 1);
> > > +		/* We let our caller release bh, so we
> > > +		 * need to duplicate the buffer before. */
> > > +		get_bh(bh);
> > > +		bforget(bh);
> > > +		unlock_buffer(bh);
> > > +	} else {
> > > +		/* Decrement the refcount only. */
> > > +		le32_add_cpu(&HDR(bh)->h_refcount, -1);
> > > +		dquot_free_block(inode, 1);
> > > +		mark_buffer_dirty(bh);
> > > +		unlock_buffer(bh);
> > > +		ea_bdebug(bh, "refcount now=%d",
> > > +			le32_to_cpu(HDR(bh)->h_refcount));
> > > +		if (IS_SYNC(inode))
> > > +			sync_dirty_buffer(bh);
> > > +	}
> > > +}
> > > +
> > >  /*
> > >   * Second half of ext2_xattr_set(): Update the file system.
> > >   */
> > > @@ -747,34 +783,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
> > >  		 * If there was an old block and we are no longer using it,
> > >  		 * release the old block.
> > >  		 */
> > > -		lock_buffer(old_bh);
> > > -		if (HDR(old_bh)->h_refcount == cpu_to_le32(1)) {
> > > -			__u32 hash = le32_to_cpu(HDR(old_bh)->h_hash);
> > > -
> > > -			/*
> > > -			 * This must happen under buffer lock for
> > > -			 * ext2_xattr_set2() to reliably detect freed block
> > > -			 */
> > > -			mb_cache_entry_delete(ea_block_cache, hash,
> > > -					      old_bh->b_blocknr);
> > > -			/* Free the old block. */
> > > -			ea_bdebug(old_bh, "freeing");
> > > -			ext2_free_blocks(inode, old_bh->b_blocknr, 1);
> > > -			mark_inode_dirty(inode);
> >
> > ^^^ this is not needed because ext2_free_blocks() will take care of it.
> > Hence you have dropped this in ext2_xattr_release_block()
>
> Correct. ext2_free_blocks() always dirties the inode (unless there is
> metadata inconsistency found in which case we don't really care).
>
> > > -			/* We let our caller release old_bh, so we
> > > -			 * need to duplicate the buffer before. */
> > > -			get_bh(old_bh);
> > > -			bforget(old_bh);
> > > -		} else {
> > > -			/* Decrement the refcount only. */
> > > -			le32_add_cpu(&HDR(old_bh)->h_refcount, -1);
> > > -			dquot_free_block_nodirty(inode, 1);
> > > -			mark_inode_dirty(inode);
> >
> > Quick qn -> Don't we need mark_inode_dirty() here?
>
> Notice that I've changed dquot_free_block_nodirty() to dquot_free_block()
> because quota info update is the only reason why we need to dirty the inode
> so why not let quota code handle it...
>

Ok, yes. Missed it. Thanks for pointing it out.

-ritesh