Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation.

[Jan Kara <jack@suse.cz>]

On Tue 24-05-11 01:08:44, Manish Katiyar wrote:
> >> @@ -449,7 +448,8 @@ ext4_xattr_set_acl(struct dentry *dentry, const
> >> char *name, const void *value,
> >>               acl = NULL;
> >>
> >>  retry:
> >> -     handle = ext4_journal_start(inode, EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
> >> +     handle = ext4_journal_start_failok(inode,
> >> +                                        EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
> >>       if (IS_ERR(handle))
> >>               return PTR_ERR(handle);
> >>       error = ext4_set_acl(handle, inode, type, acl);
> >  This change is OK. But looking at the code there, we should rather do
> > if (IS_ERR(handle)) {
> >        error = PTR_ERR(handle);
> >        goto release_and_out;
> > }
> >  Can you please include this change in your other patch fixing ACL error
> > handling? Thanks.
> 
> I already had fixed this as part of the earlier ACL patch that I
> posted, so didn't fix it here.
  I see but you should base this patch on top of all previous ones in the
series.

> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> >> index f2fa5e8..f7b2d4d 100644
> >> --- a/fs/ext4/inode.c
> >> +++ b/fs/ext4/inode.c
> >> @@ -3523,7 +3523,7 @@ retry:
> >>               int err;
> >>
> >>               /* Credits for sb + inode write */
> >> -             handle = ext4_journal_start(inode, 2);
> >> +             handle = ext4_journal_start_failok(inode, 2);
> >>               if (IS_ERR(handle)) {
> >>                       /* This is really bad luck. We've written the data
> >>                        * but cannot extend i_size. Bail out and pretend
> >  Here we shouldn't fail because that will leave blocks outside EOF
> > allocated. So just leave there original ext4_journal_start().
> 
> ohh okie... Actually for one of the similar patches earlier, you had
> suggested that it can fail, so I followed the same. Will change it to
> nofail version.
  Hmm, maybe I was wrong back then. Or wasn't it a different place?

> >> @@ -338,7 +340,7 @@ handle_t *jbd2_journal_start(journal_t *journal,
> >> int nblocks, bool errok)
> >>
> >>       current->journal_info = handle;
> >>
> >> -     err = start_this_handle(journal, handle, GFP_NOFS);
> >> +     err = start_this_handle(journal, handle, errok ? GFP_KERNEL : GFP_NOFS);
> >>       if (err < 0) {
> >>               jbd2_free_handle(handle);
> >>               current->journal_info = NULL;
> >  This is probably just a leftover from some previous version?
> 
> Actually no. I added this as part of this patch. So do I actually
> switch the gfp_mask in the last patch of the series ?
  I guess we still misunderstand about the gfp_mask :) No misuse of gfp
mask to mean whether an allocation can fail or not in any layer! If we need
to pass that information, use a separate parameter. Change all transaction
/ handle allocation gfp masks to GFP_NOFS if they are different (thus
there's no real need to pass the gfp mask). Clear now?

If something of the above is not yet done, do it in the patch where you
remove jbd2__journal_start().

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html