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.
Signed-off-by: Manish Katiyar <[email protected]>
---
fs/ext4/ext4_jbd2.h | 8 +++++++-
fs/ext4/super.c | 15 +++++++++++++--
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index d0f5353..ee6385c 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -162,6 +162,7 @@ int __ext4_handle_dirty_super(const char *where,
unsigned int line,
__ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb))
handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks);
+handle_t *ext4_journal_start_sb_tryhard(struct super_block *sb, int nblocks);
int __ext4_journal_stop(const char *where, unsigned int line,
handle_t *handle);
#define EXT4_NOJOURNAL_MAX_REF_COUNT ((unsigned long) 4096)
@@ -207,6 +208,11 @@ static inline handle_t *ext4_journal_start(struct
inode *inode, int nblocks)
return ext4_journal_start_sb(inode->i_sb, nblocks);
}
+static inline handle_t *ext4_journal_start_tryhard(struct inode
*inode, int nblocks)
+{
+ return ext4_journal_start_sb_tryhard(inode->i_sb, nblocks);
+}
+
#define ext4_journal_stop(handle) \
__ext4_journal_stop(__func__, __LINE__, (handle))
@@ -225,7 +231,7 @@ static inline int ext4_journal_extend(handle_t
*handle, int nblocks)
static inline int ext4_journal_restart(handle_t *handle, int nblocks)
{
if (ext4_handle_valid(handle))
- return jbd2_journal_restart(handle, nblocks);
+ return jbd2_journal_restart(handle, nblocks, false);
return 0;
}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8553dfb..03eac6a 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(struct super_block *sb,
+ int nblocks, bool errok)
{
journal_t *journal;
handle_t *handle;
@@ -279,7 +280,17 @@ handle_t *ext4_journal_start_sb(struct
super_block *sb, int nblocks)
ext4_abort(sb, "Detected aborted journal");
return ERR_PTR(-EROFS);
}
- return jbd2_journal_start(journal, nblocks);
+ return jbd2_journal_start(journal, nblocks, errok);
+}
+
+handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
+{
+ return __ext4_journal_start_sb(sb, nblocks, true);
+}
+
+handle_t *ext4_journal_start_sb_tryhard(struct super_block *sb, int nblocks)
+{
+ return __ext4_journal_start_sb(sb, nblocks, false);
}
/*
--
1.7.1
--
Thanks -
Manish
On Sun 24-04-11 17:13:18, Manish Katiyar wrote:
> 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. Then in the patch you can switch safe call sites to use _enomem
variant and at the end of the series as a separate patch you just do
renaming - ext4_journal_start_sb() becomes ext4_journal_start_sb_tryhard()
and ext4_journal_start_sb_enomem() becomes ext4_journal_start_sb().
Honza
> Signed-off-by: Manish Katiyar <[email protected]>
> ---
> fs/ext4/ext4_jbd2.h | 8 +++++++-
> fs/ext4/super.c | 15 +++++++++++++--
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index d0f5353..ee6385c 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -162,6 +162,7 @@ int __ext4_handle_dirty_super(const char *where,
> unsigned int line,
> __ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb))
>
> handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks);
> +handle_t *ext4_journal_start_sb_tryhard(struct super_block *sb, int nblocks);
> int __ext4_journal_stop(const char *where, unsigned int line,
> handle_t *handle);
>
> #define EXT4_NOJOURNAL_MAX_REF_COUNT ((unsigned long) 4096)
> @@ -207,6 +208,11 @@ static inline handle_t *ext4_journal_start(struct
> inode *inode, int nblocks)
> return ext4_journal_start_sb(inode->i_sb, nblocks);
> }
>
> +static inline handle_t *ext4_journal_start_tryhard(struct inode
> *inode, int nblocks)
> +{
> + return ext4_journal_start_sb_tryhard(inode->i_sb, nblocks);
> +}
> +
> #define ext4_journal_stop(handle) \
> __ext4_journal_stop(__func__, __LINE__, (handle))
>
> @@ -225,7 +231,7 @@ static inline int ext4_journal_extend(handle_t
> *handle, int nblocks)
> static inline int ext4_journal_restart(handle_t *handle, int nblocks)
> {
> if (ext4_handle_valid(handle))
> - return jbd2_journal_restart(handle, nblocks);
> + return jbd2_journal_restart(handle, nblocks, false);
> return 0;
> }
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8553dfb..03eac6a 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(struct super_block *sb,
> + int nblocks, bool errok)
> {
> journal_t *journal;
> handle_t *handle;
> @@ -279,7 +280,17 @@ handle_t *ext4_journal_start_sb(struct
> super_block *sb, int nblocks)
> ext4_abort(sb, "Detected aborted journal");
> return ERR_PTR(-EROFS);
> }
> - return jbd2_journal_start(journal, nblocks);
> + return jbd2_journal_start(journal, nblocks, errok);
> +}
> +
> +handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
> +{
> + return __ext4_journal_start_sb(sb, nblocks, true);
> +}
> +
> +handle_t *ext4_journal_start_sb_tryhard(struct super_block *sb, int nblocks)
> +{
> + return __ext4_journal_start_sb(sb, nblocks, false);
> }
>
> /*
> --
> 1.7.1
>
> --
> Thanks -
> Manish
> From 365d29cd8d5b139e332965a536dd380e656bbd15 Mon Sep 17 00:00:00 2001
> From: Manish Katiyar <[email protected]>
> Date: Sun, 24 Apr 2011 16:49:48 -0700
> Subject: [PATCH 2/5] Update low level ext4 journal routines to pass an extra paramter
> 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.
>
>
> Signed-off-by: Manish Katiyar <[email protected]>
> ---
> fs/ext4/ext4_jbd2.h | 8 +++++++-
> fs/ext4/super.c | 15 +++++++++++++--
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index d0f5353..ee6385c 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -162,6 +162,7 @@ int __ext4_handle_dirty_super(const char *where, unsigned int line,
> __ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb))
>
> handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks);
> +handle_t *ext4_journal_start_sb_tryhard(struct super_block *sb, int nblocks);
> int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle);
>
> #define EXT4_NOJOURNAL_MAX_REF_COUNT ((unsigned long) 4096)
> @@ -207,6 +208,11 @@ static inline handle_t *ext4_journal_start(struct inode *inode, int nblocks)
> return ext4_journal_start_sb(inode->i_sb, nblocks);
> }
>
> +static inline handle_t *ext4_journal_start_tryhard(struct inode *inode, int nblocks)
> +{
> + return ext4_journal_start_sb_tryhard(inode->i_sb, nblocks);
> +}
> +
> #define ext4_journal_stop(handle) \
> __ext4_journal_stop(__func__, __LINE__, (handle))
>
> @@ -225,7 +231,7 @@ static inline int ext4_journal_extend(handle_t *handle, int nblocks)
> static inline int ext4_journal_restart(handle_t *handle, int nblocks)
> {
> if (ext4_handle_valid(handle))
> - return jbd2_journal_restart(handle, nblocks);
> + return jbd2_journal_restart(handle, nblocks, false);
> return 0;
> }
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8553dfb..03eac6a 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(struct super_block *sb,
> + int nblocks, bool errok)
> {
> journal_t *journal;
> handle_t *handle;
> @@ -279,7 +280,17 @@ handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
> ext4_abort(sb, "Detected aborted journal");
> return ERR_PTR(-EROFS);
> }
> - return jbd2_journal_start(journal, nblocks);
> + return jbd2_journal_start(journal, nblocks, errok);
> +}
> +
> +handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
> +{
> + return __ext4_journal_start_sb(sb, nblocks, true);
> +}
> +
> +handle_t *ext4_journal_start_sb_tryhard(struct super_block *sb, int nblocks)
> +{
> + return __ext4_journal_start_sb(sb, nblocks, false);
> }
>
> /*
> --
> 1.7.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, May 11, 2011 at 9:04 AM, Jan Kara <[email protected]> wrote:
> On Sun 24-04-11 17:13:18, Manish Katiyar wrote:
>> 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.
Signed-off-by: Manish Katiyar <[email protected]>
---
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);
goto out;
}
error = ext4_set_acl(handle, inode, ACL_TYPE_ACCESS, clone);
@@ -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);
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 0abee1b..eae4c0a 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -161,6 +161,7 @@ int __ext4_handle_dirty_super(const char *where,
unsigned int line,
#define ext4_handle_dirty_super(handle, sb) \
__ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb))
+handle_t *ext4_journal_start_sb_failok(struct super_block *sb, int nblocks);
handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks);
int __ext4_journal_stop(const char *where, unsigned int line,
handle_t *handle);
@@ -202,7 +203,14 @@ static inline int
ext4_handle_has_enough_credits(handle_t *handle, int needed)
return 1;
}
-static inline handle_t *ext4_journal_start(struct inode *inode, int nblocks)
+static inline handle_t *ext4_journal_start_failok(struct inode *inode,
+ int nblocks)
+{
+ return ext4_journal_start_sb_failok(inode->i_sb, nblocks);
+}
+
+static inline handle_t *ext4_journal_start(struct inode *inode,
+ int nblocks)
{
return ext4_journal_start_sb(inode->i_sb, nblocks);
}
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 4890d6f..5cb3cb2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3685,7 +3685,7 @@ retry:
while (ret >= 0 && ret < max_blocks) {
map.m_lblk = map.m_lblk + ret;
map.m_len = max_blocks = max_blocks - ret;
- handle = ext4_journal_start(inode, credits);
+ handle = ext4_journal_start_failok(inode, credits);
if (IS_ERR(handle)) {
ret = PTR_ERR(handle);
break;
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
@@ -1624,7 +1624,7 @@ static int ext4_write_begin(struct file *file,
struct address_space *mapping,
to = from + len;
retry:
- handle = ext4_journal_start(inode, needed_blocks);
+ handle = ext4_journal_start_failok(inode, needed_blocks);
if (IS_ERR(handle)) {
ret = PTR_ERR(handle);
goto out;
@@ -3126,7 +3126,7 @@ retry:
* to journalling the i_disksize update if writes to the end
* of file which has an already mapped buffer.
*/
- handle = ext4_journal_start(inode, 1);
+ handle = ext4_journal_start_failok(inode, 1);
if (IS_ERR(handle)) {
ret = PTR_ERR(handle);
goto out;
@@ -3480,7 +3480,7 @@ static ssize_t ext4_ind_direct_IO(int rw, struct
kiocb *iocb,
if (final_size > inode->i_size) {
/* Credits for sb + inode write */
- handle = ext4_journal_start(inode, 2);
+ handle = ext4_journal_start_failok(inode, 2);
if (IS_ERR(handle)) {
ret = PTR_ERR(handle);
goto out;
@@ -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
@@ -5279,8 +5279,9 @@ int ext4_setattr(struct dentry *dentry, struct
iattr *attr)
/* (user+group)*(old+new) structure, inode write (sb,
* inode block, ? - but truncate inode update has it) */
- handle = ext4_journal_start(inode, (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+
- EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3);
+ handle = ext4_journal_start_failok(inode,
+ (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+
+ EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3);
if (IS_ERR(handle)) {
error = PTR_ERR(handle);
goto err_out;
@@ -5315,7 +5316,7 @@ int ext4_setattr(struct dentry *dentry, struct
iattr *attr)
(ext4_test_inode_flag(inode, EXT4_INODE_EOFBLOCKS)))) {
handle_t *handle;
- handle = ext4_journal_start(inode, 3);
+ handle = ext4_journal_start_failok(inode, 3);
if (IS_ERR(handle)) {
error = PTR_ERR(handle);
goto err_out;
@@ -5371,7 +5372,9 @@ int ext4_setattr(struct dentry *dentry, struct
iattr *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);
+ }
if (!error)
error = rc;
return error;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 808c554..4fc2630 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -101,7 +101,7 @@ long ext4_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
} else if (oldflags & EXT4_EOFBLOCKS_FL)
ext4_truncate(inode);
- handle = ext4_journal_start(inode, 1);
+ handle = ext4_journal_start_failok(inode, 1);
if (IS_ERR(handle)) {
err = PTR_ERR(handle);
goto flags_out;
@@ -157,7 +157,7 @@ flags_out:
goto setversion_out;
}
- handle = ext4_journal_start(inode, 1);
+ handle = ext4_journal_start_failok(inode, 1);
if (IS_ERR(handle)) {
err = PTR_ERR(handle);
goto setversion_out;
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
@@ -484,7 +484,7 @@ int ext4_ext_migrate(struct inode *inode)
*/
return retval;
- handle = ext4_journal_start(inode,
+ handle = ext4_journal_start_failok(inode,
EXT4_DATA_TRANS_BLOCKS(inode->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 +
EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)
@@ -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
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index b9f3e78..3afd60d 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -813,7 +813,7 @@ move_extent_per_page(struct file *o_filp, struct
inode *donor_inode,
* inode and donor_inode may change each different metadata blocks.
*/
jblocks = ext4_writepage_trans_blocks(orig_inode) * 2;
- handle = ext4_journal_start(orig_inode, jblocks);
+ handle = ext4_journal_start_failok(orig_inode, jblocks);
if (IS_ERR(handle)) {
*err = PTR_ERR(handle);
return 0;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 67fd0b0..0ad321b5 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1738,7 +1738,8 @@ static int ext4_create(struct inode *dir, struct
dentry *dentry, int mode,
dquot_initialize(dir);
retry:
- handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 +
EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb));
if (IS_ERR(handle))
@@ -1774,7 +1775,8 @@ static int ext4_mknod(struct inode *dir, struct
dentry *dentry,
dquot_initialize(dir);
retry:
- handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 +
EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb));
if (IS_ERR(handle))
@@ -1813,7 +1815,8 @@ static int ext4_mkdir(struct inode *dir, struct
dentry *dentry, int mode)
dquot_initialize(dir);
retry:
- handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 +
EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb));
if (IS_ERR(handle))
@@ -2128,7 +2131,8 @@ static int ext4_rmdir(struct inode *dir, struct
dentry *dentry)
dquot_initialize(dir);
dquot_initialize(dentry->d_inode);
- handle = ext4_journal_start(dir, EXT4_DELETE_TRANS_BLOCKS(dir->i_sb));
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DELETE_TRANS_BLOCKS(dir->i_sb));
if (IS_ERR(handle))
return PTR_ERR(handle);
@@ -2190,7 +2194,8 @@ static int ext4_unlink(struct inode *dir, struct
dentry *dentry)
dquot_initialize(dir);
dquot_initialize(dentry->d_inode);
- handle = ext4_journal_start(dir, EXT4_DELETE_TRANS_BLOCKS(dir->i_sb));
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DELETE_TRANS_BLOCKS(dir->i_sb));
if (IS_ERR(handle))
return PTR_ERR(handle);
@@ -2248,7 +2253,8 @@ static int ext4_symlink(struct inode *dir,
dquot_initialize(dir);
retry:
- handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS + 5 +
EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb));
if (IS_ERR(handle))
@@ -2308,7 +2314,8 @@ static int ext4_link(struct dentry *old_dentry,
dquot_initialize(dir);
retry:
- handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
+ handle = ext4_journal_start_failok(dir,
+ EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS);
if (IS_ERR(handle))
return PTR_ERR(handle);
@@ -2359,7 +2366,7 @@ static int ext4_rename(struct inode *old_dir,
struct dentry *old_dentry,
* in separate transaction */
if (new_dentry->d_inode)
dquot_initialize(new_dentry->d_inode);
- handle = ext4_journal_start(old_dir, 2 *
+ handle = ext4_journal_start_failok(old_dir, 2 *
EXT4_DATA_TRANS_BLOCKS(old_dir->i_sb) +
EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2);
if (IS_ERR(handle))
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)
{
journal_t *journal;
handle_t *handle;
@@ -279,7 +280,17 @@ handle_t *ext4_journal_start_sb(struct
super_block *sb, int nblocks)
ext4_abort(sb, "Detected aborted journal");
return ERR_PTR(-EROFS);
}
- return jbd2_journal_start(journal, nblocks, false);
+ return jbd2_journal_start(journal, nblocks, errok);
+}
+
+handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
+{
+ return ext4_journal_start_sb_int(sb, nblocks, false);
+}
+
+handle_t *ext4_journal_start_sb_failok(struct super_block *sb, int nblocks)
+{
+ return ext4_journal_start_sb_int(sb, nblocks, true);
}
/*
@@ -4654,7 +4665,7 @@ static int ext4_quota_off(struct super_block
*sb, int type)
/* Update modification times of quota files when userspace can
* start looking at them */
- handle = ext4_journal_start(inode, 1);
+ handle = ext4_journal_start_failok(inode, 1);
if (IS_ERR(handle))
goto out;
inode->i_mtime = inode->i_ctime = CURRENT_TIME;
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b545ca1..a4be614 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1084,7 +1084,8 @@ ext4_xattr_set(struct inode *inode, int
name_index, const char *name,
int error, retries = 0;
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)) {
error = PTR_ERR(handle);
} else {
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
@@ -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;
--
1.7.4.1
--
Thanks -
Manish
On Sat 21-05-11 19:43:10, Manish Katiyar wrote:
> On Wed, May 11, 2011 at 9:04 AM, Jan Kara <[email protected]> wrote:
> > On Sun 24-04-11 17:13:18, Manish Katiyar wrote:
> >> 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.
> Signed-off-by: Manish Katiyar <[email protected]>
> ---
> 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).
> @@ -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.
> 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().
> @@ -5371,7 +5372,9 @@ int ext4_setattr(struct dentry *dentry, struct
> iattr *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...
> 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().
> 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...
> 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...
> @@ -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 <[email protected]>
SUSE Labs, CR
On Mon, May 23, 2011 at 9:41 AM, Jan Kara <[email protected]> wrote:
> On Sat 21-05-11 19:43:10, Manish Katiyar wrote:
>> On Wed, May 11, 2011 at 9:04 AM, Jan Kara <[email protected]> wrote:
>> > On Sun 24-04-11 17:13:18, Manish Katiyar wrote:
>> >> 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.
Thanks a lot Jan,
Will send the updated patch based on your comments.
>
>> Signed-off-by: Manish Katiyar <[email protected]>
>> ---
>> ?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).
Ok.... will skip it only for -ENOMEM.
>
>> @@ -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.
>
>> 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.
>
>> @@ -5371,7 +5372,9 @@ int ext4_setattr(struct dentry *dentry, struct
>> iattr *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...
ok.
>
>> 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().
ok.
>
>> 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...
>
>> 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...
Will do.
>
>> @@ -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 ?
--
Thanks -
Manish
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 <[email protected]>
SUSE Labs, CR