On Tue 24-05-11 01:08:44, Manish Katiyar wrote:

quoted

quoted

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

quoted

quoted

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?

quoted

quoted

@@ -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 [off-list ref]
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