Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation.
From: Jan Kara <jack@suse.cz>
Date: 2011-05-23 16:41:25
On Sat 21-05-11 19:43:10, Manish Katiyar wrote:
On Wed, May 11, 2011 at 9:04 AM, Jan Kara [off-list ref] wrote:quoted
On Sun 24-04-11 17:13:18, Manish Katiyar wrote:quoted
Update low level ext4 journal routines to pass an extra parameter to journal allocation routines to specify whether transaction allocation can fail or not. With this patch ext4_journal_start() can fail due to ENOMEM. Added a new interface ext4_journal_start_tryhard() which isn't supposed to fail and keep retrying till the allocation succeeds.As I wrote in a comment in the comment to the first patch, first just make ext4_journal_start_sb() and similar functions pass false as a part of the first patch. Then it would be better to create a new function that passes true - the name does not really matter since it will be removed later in the series but it will help the review process. You can call it ext4_journal_start_sb_enomem() or whatever. This way we keep backward compatibility because currently all call sites really expect the retry behavior.Hi Jan, Here is the updated patch incorporating your comments. This adds a new function ext4_journal_start_failok and updates the ext4 code where we can fail. This patch adds a new wrapper ext4_journal_start_failok() which can fail with -ENOMEM. Update the ext4 code with this, where callers are ok failing the transaction start.
Thanks. My comments are below.
quoted hunk ↗ jump to hunk
Signed-off-by: Manish Katiyar <redacted> --- fs/ext4/acl.c | 6 +++--- fs/ext4/ext4_jbd2.h | 10 +++++++++- fs/ext4/extents.c | 2 +- fs/ext4/inode.c | 19 +++++++++++-------- fs/ext4/ioctl.c | 4 ++-- fs/ext4/migrate.c | 4 ++-- fs/ext4/move_extent.c | 2 +- fs/ext4/namei.c | 23 +++++++++++++++-------- fs/ext4/super.c | 17 ++++++++++++++--- fs/ext4/xattr.c | 3 ++- fs/jbd2/transaction.c | 4 +++- 11 files changed, 63 insertions(+), 31 deletions(-)diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index 21eacd7..cdb1f51 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c@@ -350,11 +350,10 @@ ext4_acl_chmod(struct inode *inode) int retries = 0; retry: - handle = ext4_journal_start(inode, + handle = ext4_journal_start_failok(inode, EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); if (IS_ERR(handle)) { error = PTR_ERR(handle); - ext4_std_error(inode->i_sb, error);
Here, you should rather do if (error != ENOMEM) ext4_std_error(inode->i_sb, error); We probably want to know about EIO (which is the other realistic error).
quoted hunk ↗ jump to hunk
@@ -449,7 +448,8 @@ ext4_xattr_set_acl(struct dentry *dentry, constchar *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.
quoted hunk ↗ jump to hunk
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().
quoted hunk ↗ jump to hunk
@@ -5371,7 +5372,9 @@ int ext4_setattr(struct dentry *dentry, structiattr *attr) rc = ext4_acl_chmod(inode); err_out: - ext4_std_error(inode->i_sb, error); + if (error != -ENOMEM) { + ext4_std_error(inode->i_sb, error); + }
No need for braces here...
quoted hunk ↗ jump to hunk
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c index 92816b4..8870746 100644 --- a/fs/ext4/migrate.c +++ b/fs/ext4/migrate.c@@ -533,7 +533,7 @@ int ext4_ext_migrate(struct inode *inode) ext4_set_inode_state(inode, EXT4_STATE_EXT_MIGRATE); up_read((&EXT4_I(inode)->i_data_sem)); - handle = ext4_journal_start(inode, 1); + handle = ext4_journal_start_failok(inode, 1); if (IS_ERR(handle)) { /* * It is impossible to update on-disk structures without
Here we should better not fail because we have inode on orphan list and need to eventually remove it. So just keep old ext4_journal_start().
quoted hunk ↗ jump to hunk
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 4e4c17f..2d57a57 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c@@ -247,7 +247,8 @@ static void ext4_put_nojournal(handle_t *handle) * ext4 prevents a new handle from being started by s_frozen, which * is in an upper layer. */ -handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) +static handle_t *ext4_journal_start_sb_int(struct super_block *sb, + int nblocks, bool errok)
Maybe __ext4_journal_start_sb() would be a more usual name...
quoted hunk ↗ jump to hunk
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index b5c2550..3453c29 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c@@ -308,6 +308,8 @@ static handle_t *new_handle(int nblocks) * handle_t *jbd2_journal_start() - Obtain a new handle. * @journal: Journal to start transaction on. * @nblocks: number of block buffer we might modify + * @errok : True if the transaction allocation can fail + * with ENOMEM. * * We make sure that the transaction can guarantee at least nblocks of * modified buffers in the log. We block until the log can guarantee
Move this to the patch adding the parameter...
quoted hunk ↗ jump to hunk
@@ -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? 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