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@xxxxxxx>
> > > ---
> > > 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
