* [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation. @ 2011-04-25 0:13 Manish Katiyar 2011-05-11 16:04 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Manish Katiyar @ 2011-04-25 0:13 UTC (permalink / raw) To: ext4; +Cc: Manish Katiyar, Jan Kara, Theodore Ts'o [-- Attachment #1: Type: text/plain, Size: 2921 bytes --] 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 <mkatiyar@gmail.com> --- 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 [-- Attachment #2: 0002-Update-low-level-ext4-journal-routines-to-pass-an-ex.patch --] [-- Type: text/x-patch, Size: 3083 bytes --] From 365d29cd8d5b139e332965a536dd380e656bbd15 Mon Sep 17 00:00:00 2001 From: Manish Katiyar <mkatiyar@gmail.com> 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 <mkatiyar@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation. 2011-04-25 0:13 [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation Manish Katiyar @ 2011-05-11 16:04 ` Jan Kara 2011-05-22 2:43 ` Manish Katiyar 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2011-05-11 16:04 UTC (permalink / raw) To: Manish Katiyar; +Cc: ext4, Manish Katiyar, Jan Kara, Theodore Ts'o 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 <mkatiyar@gmail.com> > --- > 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 <mkatiyar@gmail.com> > 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 <mkatiyar@gmail.com> > --- > 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 <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation. 2011-05-11 16:04 ` Jan Kara @ 2011-05-22 2:43 ` Manish Katiyar 2011-05-23 16:41 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Manish Katiyar @ 2011-05-22 2:43 UTC (permalink / raw) To: Jan Kara; +Cc: ext4, mkatiyar, Theodore Ts'o On Wed, May 11, 2011 at 9:04 AM, Jan Kara <jack@suse.cz> 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 <mkatiyar@gmail.com> --- 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 -- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation. 2011-05-22 2:43 ` Manish Katiyar @ 2011-05-23 16:41 ` Jan Kara 2011-05-24 8:08 ` Manish Katiyar 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2011-05-23 16:41 UTC (permalink / raw) To: Manish Katiyar; +Cc: Jan Kara, ext4, Theodore Ts'o On Sat 21-05-11 19:43:10, Manish Katiyar wrote: > On Wed, May 11, 2011 at 9:04 AM, Jan Kara <jack@suse.cz> 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 <mkatiyar@gmail.com> > --- > 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 <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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation. 2011-05-23 16:41 ` Jan Kara @ 2011-05-24 8:08 ` Manish Katiyar 2011-05-24 10:13 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Manish Katiyar @ 2011-05-24 8:08 UTC (permalink / raw) To: Jan Kara; +Cc: ext4, Theodore Ts'o On Mon, May 23, 2011 at 9:41 AM, Jan Kara <jack@suse.cz> wrote: > On Sat 21-05-11 19:43:10, Manish Katiyar wrote: >> On Wed, May 11, 2011 at 9:04 AM, Jan Kara <jack@suse.cz> 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 <mkatiyar@gmail.com> >> --- >> 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 -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation. 2011-05-24 8:08 ` Manish Katiyar @ 2011-05-24 10:13 ` Jan Kara 0 siblings, 0 replies; 6+ messages in thread From: Jan Kara @ 2011-05-24 10:13 UTC (permalink / raw) To: Manish Katiyar; +Cc: Jan Kara, ext4, Theodore Ts'o 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-24 10:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-25 0:13 [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation Manish Katiyar 2011-05-11 16:04 ` Jan Kara 2011-05-22 2:43 ` Manish Katiyar 2011-05-23 16:41 ` Jan Kara 2011-05-24 8:08 ` Manish Katiyar 2011-05-24 10:13 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).