* [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures @ 2011-01-23 3:34 Manish Katiyar 2011-01-25 16:17 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Manish Katiyar @ 2011-01-23 3:34 UTC (permalink / raw) To: Jan Kara, ext4 Pass GFP_KERNEL for transaction allocation for ext4 routines if caller can handler failures Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> --- fs/ext4/acl.c | 5 +++-- fs/ext4/ext4_jbd2.h | 9 +++++---- fs/ext4/extents.c | 8 ++++---- fs/ext4/ialloc.c | 2 +- fs/ext4/inode.c | 33 ++++++++++++++++++--------------- fs/ext4/ioctl.c | 4 ++-- fs/ext4/migrate.c | 4 ++-- fs/ext4/move_extent.c | 2 +- fs/ext4/namei.c | 24 +++++++++++++++--------- fs/ext4/resize.c | 8 ++++---- fs/ext4/super.c | 13 +++++++------ fs/ext4/xattr.c | 3 ++- 12 files changed, 64 insertions(+), 51 deletions(-) diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index e0270d1..1a4a944 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -351,7 +351,7 @@ ext4_acl_chmod(struct inode *inode) retry: handle = ext4_journal_start(inode, - EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); if (IS_ERR(handle)) { error = PTR_ERR(handle); ext4_std_error(inode->i_sb, error); @@ -449,7 +449,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(inode, + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); 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 d8b992e..38f128e 100644 --- a/fs/ext4/ext4_jbd2.h +++ b/fs/ext4/ext4_jbd2.h @@ -161,7 +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(struct super_block *sb, int nblocks); +handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks, bool errok); int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle); #define EXT4_NOJOURNAL_MAX_REF_COUNT ((unsigned long) 4096) @@ -209,9 +209,10 @@ static inline void ext4_journal_release_buffer(handle_t *handle, jbd2_journal_release_buffer(handle, bh); } -static inline handle_t *ext4_journal_start(struct inode *inode, int nblocks) +static inline handle_t *ext4_journal_start(struct inode *inode, + int nblocks, bool errok) { - return ext4_journal_start_sb(inode->i_sb, nblocks); + return ext4_journal_start_sb(inode->i_sb, nblocks, errok); } #define ext4_journal_stop(handle) \ @@ -232,7 +233,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/extents.c b/fs/ext4/extents.c index 63a7581..5944b1c 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -2333,7 +2333,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start) ext_debug("truncate since %u\n", start); /* probably first extent we're gonna free will be last in block */ - handle = ext4_journal_start(inode, depth + 1); + handle = ext4_journal_start(inode, depth + 1, false); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -3544,7 +3544,7 @@ void ext4_ext_truncate(struct inode *inode) * probably first extent we're gonna free will be last in block */ err = ext4_writepage_trans_blocks(inode); - handle = ext4_journal_start(inode, err); + handle = ext4_journal_start(inode, err, false); if (IS_ERR(handle)) return; @@ -3677,7 +3677,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(inode, credits, true); if (IS_ERR(handle)) { ret = PTR_ERR(handle); break; @@ -3752,7 +3752,7 @@ int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset, while (ret >= 0 && ret < max_blocks) { map.m_lblk += ret; map.m_len = (max_blocks -= ret); - handle = ext4_journal_start(inode, credits); + handle = ext4_journal_start(inode, credits, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); break; diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index eb9097a..e0e27a3 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -1257,7 +1257,7 @@ extern int ext4_init_inode_table(struct super_block *sb, ext4_group_t group, if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_ZEROED)) goto out; - handle = ext4_journal_start_sb(sb, 1); + handle = ext4_journal_start_sb(sb, 1, true); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 9f7f9e4..76c20b8 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -129,7 +129,7 @@ static handle_t *start_transaction(struct inode *inode) { handle_t *result; - result = ext4_journal_start(inode, blocks_for_truncate(inode)); + result = ext4_journal_start(inode, blocks_for_truncate(inode), false); if (!IS_ERR(result)) return result; @@ -204,7 +204,7 @@ void ext4_evict_inode(struct inode *inode) if (is_bad_inode(inode)) goto no_delete; - handle = ext4_journal_start(inode, blocks_for_truncate(inode)+3); + handle = ext4_journal_start(inode, blocks_for_truncate(inode)+3, false); if (IS_ERR(handle)) { ext4_std_error(inode->i_sb, PTR_ERR(handle)); /* @@ -1398,7 +1398,7 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock, if (map.m_len > DIO_MAX_BLOCKS) map.m_len = DIO_MAX_BLOCKS; dio_credits = ext4_chunk_trans_blocks(inode, map.m_len); - handle = ext4_journal_start(inode, dio_credits); + handle = ext4_journal_start(inode, dio_credits, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); return ret; @@ -1621,7 +1621,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(inode, needed_blocks, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -2653,7 +2653,8 @@ static int __ext4_journalled_writepage(struct page *page, * references to buffers so we are safe */ unlock_page(page); - handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode)); + handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode), + false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -3049,7 +3050,7 @@ retry: needed_blocks = ext4_da_writepages_trans_blocks(inode); /* start a new transaction*/ - handle = ext4_journal_start(inode, needed_blocks); + handle = ext4_journal_start(inode, needed_blocks, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); ext4_msg(inode->i_sb, KERN_CRIT, "%s: jbd2_start: " @@ -3204,7 +3205,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(inode, 1, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -3553,7 +3554,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(inode, 2, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -3596,7 +3597,7 @@ retry: int err; /* Credits for sb + inode write */ - handle = ext4_journal_start(inode, 2); + handle = ext4_journal_start(inode, 2, false); if (IS_ERR(handle)) { /* This is really bad luck. We've written the data * but cannot extend i_size. Bail out and pretend @@ -5329,8 +5330,10 @@ 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(inode, + (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+ + EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3, + true); if (IS_ERR(handle)) { error = PTR_ERR(handle); goto err_out; @@ -5365,7 +5368,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(inode, 3, true); if (IS_ERR(handle)) { error = PTR_ERR(handle); goto err_out; @@ -5385,7 +5388,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) attr->ia_size); if (error) { /* Do as much error cleanup as possible */ - handle = ext4_journal_start(inode, 3); + handle = ext4_journal_start(inode, 3, false); if (IS_ERR(handle)) { ext4_orphan_del(NULL, inode); goto err_out; @@ -5738,7 +5741,7 @@ void ext4_dirty_inode(struct inode *inode) { handle_t *handle; - handle = ext4_journal_start(inode, 2); + handle = ext4_journal_start(inode, 2, false); if (IS_ERR(handle)) goto out; @@ -5822,7 +5825,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val) /* Finally we can mark the inode as dirty. */ - handle = ext4_journal_start(inode, 1); + handle = ext4_journal_start(inode, 1, true); if (IS_ERR(handle)) return PTR_ERR(handle); diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index eb3bc2f..3e7977b 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(inode, 1, false); 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(inode, 1, true); if (IS_ERR(handle)) { err = PTR_ERR(handle); goto setversion_out; diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c index b0a126f..729dcbc 100644 --- a/fs/ext4/migrate.c +++ b/fs/ext4/migrate.c @@ -488,7 +488,7 @@ int ext4_ext_migrate(struct inode *inode) EXT4_DATA_TRANS_BLOCKS(inode->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb) - + 1); + + 1, true); if (IS_ERR(handle)) { retval = PTR_ERR(handle); return retval; @@ -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(inode, 1, true); 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..d5aad4d 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(orig_inode, jblocks, true); if (IS_ERR(handle)) { *err = PTR_ERR(handle); return 0; diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 5485390..a3ad11f 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1739,7 +1739,8 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, int mode, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -1775,7 +1776,8 @@ static int ext4_mknod(struct inode *dir, struct dentry *dentry, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -1814,7 +1816,8 @@ static int ext4_mkdir(struct inode *dir, struct dentry *dentry, int mode) retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2127,7 +2130,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(dir, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb), true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2188,7 +2192,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(dir, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb), true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2246,8 +2251,9 @@ static int ext4_symlink(struct inode *dir, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 5 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 5 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2313,7 +2319,7 @@ static int ext4_link(struct dentry *old_dentry, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS); + EXT4_INDEX_EXTRA_TRANS_BLOCKS, true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2365,7 +2371,7 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, dquot_initialize(new_dentry->d_inode); handle = ext4_journal_start(old_dir, 2 * EXT4_DATA_TRANS_BLOCKS(old_dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2); + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2, true); if (IS_ERR(handle)) return PTR_ERR(handle); diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c index 3ecc6e4..e50d083 100644 --- a/fs/ext4/resize.c +++ b/fs/ext4/resize.c @@ -176,7 +176,7 @@ static int setup_new_group_blocks(struct super_block *sb, int err = 0, err2; /* This transaction may be extended/restarted along the way */ - handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA); + handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA, true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -655,7 +655,7 @@ static void update_backups(struct super_block *sb, handle_t *handle; int err = 0, err2; - handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA); + handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA, true); if (IS_ERR(handle)) { group = 1; err = PTR_ERR(handle); @@ -793,7 +793,7 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input) */ handle = ext4_journal_start_sb(sb, ext4_bg_has_super(sb, input->group) ? - 3 + reserved_gdb : 4); + 3 + reserved_gdb : 4, true); if (IS_ERR(handle)) { err = PTR_ERR(handle); goto exit_put; @@ -1031,7 +1031,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es, /* We will update the superblock, one block bitmap, and * one group descriptor via ext4_free_blocks(). */ - handle = ext4_journal_start_sb(sb, 3); + handle = ext4_journal_start_sb(sb, 3, true); if (IS_ERR(handle)) { err = PTR_ERR(handle); ext4_warning(sb, "error %d on journal start", err); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index cb10a06..7714a15 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -241,7 +241,7 @@ static void ext4_put_nojournal(handle_t *handle) * that sync() will call the filesystem's write_super callback if * appropriate. */ -handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) +handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks, bool errok) { journal_t *journal; @@ -258,7 +258,7 @@ 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); } return ext4_get_nojournal(); } @@ -4471,7 +4471,8 @@ static int ext4_write_dquot(struct dquot *dquot) inode = dquot_to_inode(dquot); handle = ext4_journal_start(inode, - EXT4_QUOTA_TRANS_BLOCKS(dquot->dq_sb)); + EXT4_QUOTA_TRANS_BLOCKS(dquot->dq_sb), + false); if (IS_ERR(handle)) return PTR_ERR(handle); ret = dquot_commit(dquot); @@ -4487,7 +4488,7 @@ static int ext4_acquire_dquot(struct dquot *dquot) handle_t *handle; handle = ext4_journal_start(dquot_to_inode(dquot), - EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb)); + EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb), true); if (IS_ERR(handle)) return PTR_ERR(handle); ret = dquot_acquire(dquot); @@ -4503,7 +4504,7 @@ static int ext4_release_dquot(struct dquot *dquot) handle_t *handle; handle = ext4_journal_start(dquot_to_inode(dquot), - EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb)); + EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb), true); if (IS_ERR(handle)) { /* Release dquot anyway to avoid endless cycle in dqput() */ dquot_release(dquot); @@ -4534,7 +4535,7 @@ static int ext4_write_info(struct super_block *sb, int type) handle_t *handle; /* Data block + inode block */ - handle = ext4_journal_start(sb->s_root->d_inode, 2); + handle = ext4_journal_start(sb->s_root->d_inode, 2, false); if (IS_ERR(handle)) return PTR_ERR(handle); ret = dquot_commit_info(sb, type); diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index fc32176..0e39f57 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(inode, + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); if (IS_ERR(handle)) { error = PTR_ERR(handle); } else { -- 1.6.0.4 -- Thanks - Manish ================================== [$\*.^ -- I miss being one of them ================================== ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-23 3:34 [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures Manish Katiyar @ 2011-01-25 16:17 ` Jan Kara 2011-01-25 20:05 ` Manish Katiyar 2011-01-30 5:29 ` Manish Katiyar 0 siblings, 2 replies; 10+ messages in thread From: Jan Kara @ 2011-01-25 16:17 UTC (permalink / raw) To: Manish Katiyar; +Cc: Jan Kara, ext4 Hi, On Sat 22-01-11 19:34:55, Manish Katiyar wrote: > Pass GFP_KERNEL for transaction allocation for ext4 routines if caller > can handler failures Some error recovery paths will need cleaning up before you actually start using them - see below: > diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c > index e0270d1..1a4a944 100644 > --- a/fs/ext4/acl.c > +++ b/fs/ext4/acl.c > @@ -351,7 +351,7 @@ ext4_acl_chmod(struct inode *inode) > > retry: > handle = ext4_journal_start(inode, > - EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); > + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); > if (IS_ERR(handle)) { > error = PTR_ERR(handle); > ext4_std_error(inode->i_sb, error); We shouldn't call ext4_std_error() (that possibly logs the message in the kernel log and remounts the fs read-only, panics the kernel or so) in case of ENOMEM... > @@ -449,7 +449,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(inode, > + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); > if (IS_ERR(handle)) > return PTR_ERR(handle); It's actually not your bug, but the above should be: error = PTR_ERR(handle); goto release_and_out; > error = ext4_set_acl(handle, inode, type, acl); > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index eb9097a..e0e27a3 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -1257,7 +1257,7 @@ extern int ext4_init_inode_table(struct > super_block *sb, ext4_group_t group, > if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_ZEROED)) > goto out; > > - handle = ext4_journal_start_sb(sb, 1); > + handle = ext4_journal_start_sb(sb, 1, true); > if (IS_ERR(handle)) { > ret = PTR_ERR(handle); > goto out; Well, this might be disputable. This function is used to lazily initialize inode table. If the initialization fails, thread removes the request for initialization from the queue. But in case of ENOMEM, it might be more suitable to just postpone the initialization work to a more suitable time... > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 9f7f9e4..76c20b8 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c ... > @@ -1398,7 +1398,7 @@ static int _ext4_get_block(struct inode *inode, > sector_t iblock, > if (map.m_len > DIO_MAX_BLOCKS) > map.m_len = DIO_MAX_BLOCKS; > dio_credits = ext4_chunk_trans_blocks(inode, map.m_len); > - handle = ext4_journal_start(inode, dio_credits); > + handle = ext4_journal_start(inode, dio_credits, false); > if (IS_ERR(handle)) { > ret = PTR_ERR(handle); > return ret; Hmm, this would be actually another useful prerequisite cleanup of this series. _ext4_get_block() should need to start a transaction only when called from direct IO path (otherwise transaction should be already started when creating blocks). But this is only implicit so it would be good to create ext4_get_block_directIO() which would start a transaction, use it as a callback of __blockdev_direct_IO(), and remove the code from standard _ext4_get_block() function. Then you can also make ext4_journal_start() possibly fail and still it will be clear you do not introduce any potential problems. > @@ -1621,7 +1621,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(inode, needed_blocks, false); > if (IS_ERR(handle)) { > ret = PTR_ERR(handle); > goto out; Failing this with ENOMEM is OK. Note that grab_cache_page_write_begin() called just below can fail with ENOMEM as well. > @@ -3553,7 +3554,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(inode, 2, false); > if (IS_ERR(handle)) { > ret = PTR_ERR(handle); > goto out; This can fail without introducing problems. It's standard directIO write path. > @@ -3596,7 +3597,7 @@ retry: > int err; > > /* Credits for sb + inode write */ > - handle = ext4_journal_start(inode, 2); > + handle = ext4_journal_start(inode, 2, false); > if (IS_ERR(handle)) { > /* This is really bad luck. We've written the data > * but cannot extend i_size. Bail out and pretend This one can fail just fine as well. > @@ -5329,8 +5330,10 @@ 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(inode, > + (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+ > + EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3, > + true); > if (IS_ERR(handle)) { > error = PTR_ERR(handle); > goto err_out; > @@ -5365,7 +5368,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(inode, 3, true); > if (IS_ERR(handle)) { > error = PTR_ERR(handle); > goto err_out; The above two sites are fine but note that err_out calls ext4_std_error() which we don't want to happen in case of ENOMEM. > @@ -5822,7 +5825,7 @@ int ext4_change_inode_journal_flag(struct inode > *inode, int val) > > /* Finally we can mark the inode as dirty. */ > > - handle = ext4_journal_start(inode, 1); > + handle = ext4_journal_start(inode, 1, true); > if (IS_ERR(handle)) > return PTR_ERR(handle); This can fail OK, but you should undo inode flag and aops change before returning error (that would be probably better as a separate preparatory patch because it won't be completely trivial - you need to lock the updates again etc. possibly create a helper function for that so that you don't duplicate the code). > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index eb3bc2f..3e7977b 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(inode, 1, false); > if (IS_ERR(handle)) { > err = PTR_ERR(handle); > goto flags_out; This can handle failure just fine... > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index cb10a06..7714a15 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -4487,7 +4488,7 @@ static int ext4_acquire_dquot(struct dquot *dquot) > handle_t *handle; > > handle = ext4_journal_start(dquot_to_inode(dquot), > - EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb)); > + EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb), true); > if (IS_ERR(handle)) > return PTR_ERR(handle); > ret = dquot_acquire(dquot); > @@ -4503,7 +4504,7 @@ static int ext4_release_dquot(struct dquot *dquot) > handle_t *handle; > > handle = ext4_journal_start(dquot_to_inode(dquot), > - EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb)); > + EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb), true); > if (IS_ERR(handle)) { > /* Release dquot anyway to avoid endless cycle in dqput() */ > dquot_release(dquot); For now put 'false' in these quota functions. Because failure here results in a failure of dquot_initialize() which is not tested in most places and thus results in quota miscomputations... Properly handling this would require another set of cleanups. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-25 16:17 ` Jan Kara @ 2011-01-25 20:05 ` Manish Katiyar 2011-01-26 7:55 ` Lukas Czerner 2011-01-30 19:42 ` Ted Ts'o 2011-01-30 5:29 ` Manish Katiyar 1 sibling, 2 replies; 10+ messages in thread From: Manish Katiyar @ 2011-01-25 20:05 UTC (permalink / raw) To: Jan Kara; +Cc: ext4 Thanks a lot Jan, I will have a look at the functions you mentioned and send an updated version. On Tue, Jan 25, 2011 at 8:17 AM, Jan Kara <jack@suse.cz> wrote: > Hi, > > On Sat 22-01-11 19:34:55, Manish Katiyar wrote: >> Pass GFP_KERNEL for transaction allocation for ext4 routines if caller >> can handler failures > Some error recovery paths will need cleaning up before you actually start > using them - see below: > >> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c >> index e0270d1..1a4a944 100644 >> --- a/fs/ext4/acl.c >> +++ b/fs/ext4/acl.c >> @@ -351,7 +351,7 @@ ext4_acl_chmod(struct inode *inode) >> >> retry: >> handle = ext4_journal_start(inode, >> - EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); >> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); >> if (IS_ERR(handle)) { >> error = PTR_ERR(handle); >> ext4_std_error(inode->i_sb, error); > We shouldn't call ext4_std_error() (that possibly logs the message in > the kernel log and remounts the fs read-only, panics the kernel or so) in > case of ENOMEM... > >> @@ -449,7 +449,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(inode, >> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); >> if (IS_ERR(handle)) >> return PTR_ERR(handle); > It's actually not your bug, but the above should be: > error = PTR_ERR(handle); > goto release_and_out; > >> error = ext4_set_acl(handle, inode, type, acl); >> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c >> index eb9097a..e0e27a3 100644 >> --- a/fs/ext4/ialloc.c >> +++ b/fs/ext4/ialloc.c >> @@ -1257,7 +1257,7 @@ extern int ext4_init_inode_table(struct >> super_block *sb, ext4_group_t group, >> if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_ZEROED)) >> goto out; >> >> - handle = ext4_journal_start_sb(sb, 1); >> + handle = ext4_journal_start_sb(sb, 1, true); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> goto out; > Well, this might be disputable. This function is used to lazily > initialize inode table. If the initialization fails, thread removes the > request for initialization from the queue. But in case of ENOMEM, it might > be more suitable to just postpone the initialization work to a more > suitable time... > >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index 9f7f9e4..76c20b8 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c > ... >> @@ -1398,7 +1398,7 @@ static int _ext4_get_block(struct inode *inode, >> sector_t iblock, >> if (map.m_len > DIO_MAX_BLOCKS) >> map.m_len = DIO_MAX_BLOCKS; >> dio_credits = ext4_chunk_trans_blocks(inode, map.m_len); >> - handle = ext4_journal_start(inode, dio_credits); >> + handle = ext4_journal_start(inode, dio_credits, false); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> return ret; > Hmm, this would be actually another useful prerequisite cleanup of this > series. _ext4_get_block() should need to start a transaction only when > called from direct IO path (otherwise transaction should be already started > when creating blocks). But this is only implicit so it would be good to > create ext4_get_block_directIO() which would start a transaction, use it > as a callback of __blockdev_direct_IO(), and remove the code from standard > _ext4_get_block() function. Then you can also make ext4_journal_start() > possibly fail and still it will be clear you do not introduce any potential > problems. > >> @@ -1621,7 +1621,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(inode, needed_blocks, false); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> goto out; > Failing this with ENOMEM is OK. Note that grab_cache_page_write_begin() > called just below can fail with ENOMEM as well. > >> @@ -3553,7 +3554,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(inode, 2, false); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> goto out; > This can fail without introducing problems. It's standard directIO write > path. > >> @@ -3596,7 +3597,7 @@ retry: >> int err; >> >> /* Credits for sb + inode write */ >> - handle = ext4_journal_start(inode, 2); >> + handle = ext4_journal_start(inode, 2, false); >> if (IS_ERR(handle)) { >> /* This is really bad luck. We've written the data >> * but cannot extend i_size. Bail out and pretend > This one can fail just fine as well. > >> @@ -5329,8 +5330,10 @@ 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(inode, >> + (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+ >> + EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3, >> + true); >> if (IS_ERR(handle)) { >> error = PTR_ERR(handle); >> goto err_out; >> @@ -5365,7 +5368,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(inode, 3, true); >> if (IS_ERR(handle)) { >> error = PTR_ERR(handle); >> goto err_out; > The above two sites are fine but note that err_out calls ext4_std_error() > which we don't want to happen in case of ENOMEM. > >> @@ -5822,7 +5825,7 @@ int ext4_change_inode_journal_flag(struct inode >> *inode, int val) >> >> /* Finally we can mark the inode as dirty. */ >> >> - handle = ext4_journal_start(inode, 1); >> + handle = ext4_journal_start(inode, 1, true); >> if (IS_ERR(handle)) >> return PTR_ERR(handle); > This can fail OK, but you should undo inode flag and aops change before > returning error (that would be probably better as a separate preparatory > patch because it won't be completely trivial - you need to lock the updates > again etc. possibly create a helper function for that so that you don't > duplicate the code). > >> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c >> index eb3bc2f..3e7977b 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(inode, 1, false); >> if (IS_ERR(handle)) { >> err = PTR_ERR(handle); >> goto flags_out; > This can handle failure just fine... I wasn't sure about this since this was calling ext4_truncate if the old_flags had EXT4_EOFBLOCKS_FL. And then in ext4_truncate() had start_transaction which was passing false. > >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index cb10a06..7714a15 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -4487,7 +4488,7 @@ static int ext4_acquire_dquot(struct dquot *dquot) >> handle_t *handle; >> >> handle = ext4_journal_start(dquot_to_inode(dquot), >> - EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb)); >> + EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb), true); >> if (IS_ERR(handle)) >> return PTR_ERR(handle); >> ret = dquot_acquire(dquot); >> @@ -4503,7 +4504,7 @@ static int ext4_release_dquot(struct dquot *dquot) >> handle_t *handle; >> >> handle = ext4_journal_start(dquot_to_inode(dquot), >> - EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb)); >> + EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb), true); >> if (IS_ERR(handle)) { >> /* Release dquot anyway to avoid endless cycle in dqput() */ >> dquot_release(dquot); > For now put 'false' in these quota functions. Because failure here > results in a failure of dquot_initialize() which is not tested in most > places and thus results in quota miscomputations... Properly handling this > would require another set of cleanups. > > Honza > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR > -- 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] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-25 20:05 ` Manish Katiyar @ 2011-01-26 7:55 ` Lukas Czerner 2011-01-26 7:58 ` Manish Katiyar 2011-01-30 19:42 ` Ted Ts'o 1 sibling, 1 reply; 10+ messages in thread From: Lukas Czerner @ 2011-01-26 7:55 UTC (permalink / raw) To: Manish Katiyar; +Cc: Jan Kara, ext4 [-- Attachment #1: Type: TEXT/PLAIN, Size: 10342 bytes --] Hi Manish, I might be very helpful to also improve commit description, because as it is it's very confusing and shallow, at least for me. Thanks! -Lukas On Tue, 25 Jan 2011, Manish Katiyar wrote: > Thanks a lot Jan, I will have a look at the functions you mentioned > and send an updated version. > > On Tue, Jan 25, 2011 at 8:17 AM, Jan Kara <jack@suse.cz> wrote: > > Hi, > > > > On Sat 22-01-11 19:34:55, Manish Katiyar wrote: > >> Pass GFP_KERNEL for transaction allocation for ext4 routines if caller > >> can handler failures > > Some error recovery paths will need cleaning up before you actually start > > using them - see below: > > > >> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c > >> index e0270d1..1a4a944 100644 > >> --- a/fs/ext4/acl.c > >> +++ b/fs/ext4/acl.c > >> @@ -351,7 +351,7 @@ ext4_acl_chmod(struct inode *inode) > >> > >> retry: > >> handle = ext4_journal_start(inode, > >> - EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); > >> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); > >> if (IS_ERR(handle)) { > >> error = PTR_ERR(handle); > >> ext4_std_error(inode->i_sb, error); > > We shouldn't call ext4_std_error() (that possibly logs the message in > > the kernel log and remounts the fs read-only, panics the kernel or so) in > > case of ENOMEM... > > > >> @@ -449,7 +449,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(inode, > >> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); > >> if (IS_ERR(handle)) > >> return PTR_ERR(handle); > > It's actually not your bug, but the above should be: > > error = PTR_ERR(handle); > > goto release_and_out; > > > >> error = ext4_set_acl(handle, inode, type, acl); > >> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > >> index eb9097a..e0e27a3 100644 > >> --- a/fs/ext4/ialloc.c > >> +++ b/fs/ext4/ialloc.c > >> @@ -1257,7 +1257,7 @@ extern int ext4_init_inode_table(struct > >> super_block *sb, ext4_group_t group, > >> if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_ZEROED)) > >> goto out; > >> > >> - handle = ext4_journal_start_sb(sb, 1); > >> + handle = ext4_journal_start_sb(sb, 1, true); > >> if (IS_ERR(handle)) { > >> ret = PTR_ERR(handle); > >> goto out; > > Well, this might be disputable. This function is used to lazily > > initialize inode table. If the initialization fails, thread removes the > > request for initialization from the queue. But in case of ENOMEM, it might > > be more suitable to just postpone the initialization work to a more > > suitable time... > > > >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > >> index 9f7f9e4..76c20b8 100644 > >> --- a/fs/ext4/inode.c > >> +++ b/fs/ext4/inode.c > > ... > >> @@ -1398,7 +1398,7 @@ static int _ext4_get_block(struct inode *inode, > >> sector_t iblock, > >> if (map.m_len > DIO_MAX_BLOCKS) > >> map.m_len = DIO_MAX_BLOCKS; > >> dio_credits = ext4_chunk_trans_blocks(inode, map.m_len); > >> - handle = ext4_journal_start(inode, dio_credits); > >> + handle = ext4_journal_start(inode, dio_credits, false); > >> if (IS_ERR(handle)) { > >> ret = PTR_ERR(handle); > >> return ret; > > Hmm, this would be actually another useful prerequisite cleanup of this > > series. _ext4_get_block() should need to start a transaction only when > > called from direct IO path (otherwise transaction should be already started > > when creating blocks). But this is only implicit so it would be good to > > create ext4_get_block_directIO() which would start a transaction, use it > > as a callback of __blockdev_direct_IO(), and remove the code from standard > > _ext4_get_block() function. Then you can also make ext4_journal_start() > > possibly fail and still it will be clear you do not introduce any potential > > problems. > > > >> @@ -1621,7 +1621,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(inode, needed_blocks, false); > >> if (IS_ERR(handle)) { > >> ret = PTR_ERR(handle); > >> goto out; > > Failing this with ENOMEM is OK. Note that grab_cache_page_write_begin() > > called just below can fail with ENOMEM as well. > > > >> @@ -3553,7 +3554,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(inode, 2, false); > >> if (IS_ERR(handle)) { > >> ret = PTR_ERR(handle); > >> goto out; > > This can fail without introducing problems. It's standard directIO write > > path. > > > >> @@ -3596,7 +3597,7 @@ retry: > >> int err; > >> > >> /* Credits for sb + inode write */ > >> - handle = ext4_journal_start(inode, 2); > >> + handle = ext4_journal_start(inode, 2, false); > >> if (IS_ERR(handle)) { > >> /* This is really bad luck. We've written the data > >> * but cannot extend i_size. Bail out and pretend > > This one can fail just fine as well. > > > >> @@ -5329,8 +5330,10 @@ 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(inode, > >> + (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+ > >> + EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3, > >> + true); > >> if (IS_ERR(handle)) { > >> error = PTR_ERR(handle); > >> goto err_out; > >> @@ -5365,7 +5368,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(inode, 3, true); > >> if (IS_ERR(handle)) { > >> error = PTR_ERR(handle); > >> goto err_out; > > The above two sites are fine but note that err_out calls ext4_std_error() > > which we don't want to happen in case of ENOMEM. > > > >> @@ -5822,7 +5825,7 @@ int ext4_change_inode_journal_flag(struct inode > >> *inode, int val) > >> > >> /* Finally we can mark the inode as dirty. */ > >> > >> - handle = ext4_journal_start(inode, 1); > >> + handle = ext4_journal_start(inode, 1, true); > >> if (IS_ERR(handle)) > >> return PTR_ERR(handle); > > This can fail OK, but you should undo inode flag and aops change before > > returning error (that would be probably better as a separate preparatory > > patch because it won't be completely trivial - you need to lock the updates > > again etc. possibly create a helper function for that so that you don't > > duplicate the code). > > > >> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > >> index eb3bc2f..3e7977b 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(inode, 1, false); > >> if (IS_ERR(handle)) { > >> err = PTR_ERR(handle); > >> goto flags_out; > > This can handle failure just fine... > > I wasn't sure about this since this was calling ext4_truncate if the > old_flags had EXT4_EOFBLOCKS_FL. And then in ext4_truncate() had > start_transaction which was passing false. > > > > > >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c > >> index cb10a06..7714a15 100644 > >> --- a/fs/ext4/super.c > >> +++ b/fs/ext4/super.c > >> @@ -4487,7 +4488,7 @@ static int ext4_acquire_dquot(struct dquot *dquot) > >> handle_t *handle; > >> > >> handle = ext4_journal_start(dquot_to_inode(dquot), > >> - EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb)); > >> + EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb), true); > >> if (IS_ERR(handle)) > >> return PTR_ERR(handle); > >> ret = dquot_acquire(dquot); > >> @@ -4503,7 +4504,7 @@ static int ext4_release_dquot(struct dquot *dquot) > >> handle_t *handle; > >> > >> handle = ext4_journal_start(dquot_to_inode(dquot), > >> - EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb)); > >> + EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb), true); > >> if (IS_ERR(handle)) { > >> /* Release dquot anyway to avoid endless cycle in dqput() */ > >> dquot_release(dquot); > > For now put 'false' in these quota functions. Because failure here > > results in a failure of dquot_initialize() which is not tested in most > > places and thus results in quota miscomputations... Properly handling this > > would require another set of cleanups. > > > > Honza > > -- > > Jan Kara <jack@suse.cz> > > SUSE Labs, CR > > > > > > -- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-26 7:55 ` Lukas Czerner @ 2011-01-26 7:58 ` Manish Katiyar 0 siblings, 0 replies; 10+ messages in thread From: Manish Katiyar @ 2011-01-26 7:58 UTC (permalink / raw) To: Lukas Czerner; +Cc: Jan Kara, ext4 On Tue, Jan 25, 2011 at 11:55 PM, Lukas Czerner <lczerner@redhat.com> wrote: > Hi Manish, > > I might be very helpful to also improve commit description, because as > it is it's very confusing and shallow, at least for me. Sure, will update that too when I send the updated version. -- Thanks - Manish ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-25 20:05 ` Manish Katiyar 2011-01-26 7:55 ` Lukas Czerner @ 2011-01-30 19:42 ` Ted Ts'o 2011-01-30 19:44 ` Manish Katiyar 1 sibling, 1 reply; 10+ messages in thread From: Ted Ts'o @ 2011-01-30 19:42 UTC (permalink / raw) To: Manish Katiyar; +Cc: Jan Kara, ext4 On Tue, Jan 25, 2011 at 12:05:07PM -0800, Manish Katiyar wrote: > Thanks a lot Jan, I will have a look at the functions you mentioned > and send an updated version. Please do the cleanups as separate patches, each with a separate commit description. Thanks!! - Ted ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-30 19:42 ` Ted Ts'o @ 2011-01-30 19:44 ` Manish Katiyar 2011-01-30 20:07 ` Ted Ts'o 0 siblings, 1 reply; 10+ messages in thread From: Manish Katiyar @ 2011-01-30 19:44 UTC (permalink / raw) To: Ted Ts'o; +Cc: Jan Kara, ext4 On Sun, Jan 30, 2011 at 11:42 AM, Ted Ts'o <tytso@mit.edu> wrote: > On Tue, Jan 25, 2011 at 12:05:07PM -0800, Manish Katiyar wrote: >> Thanks a lot Jan, I will have a look at the functions you mentioned >> and send an updated version. > > Please do the cleanups as separate patches, each with a separate > commit description. Hi Ted, Do you mean separate patch for each of the changed functions ? -- Thanks - Manish ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-30 19:44 ` Manish Katiyar @ 2011-01-30 20:07 ` Ted Ts'o 2011-01-31 1:04 ` Manish Katiyar 0 siblings, 1 reply; 10+ messages in thread From: Ted Ts'o @ 2011-01-30 20:07 UTC (permalink / raw) To: Manish Katiyar; +Cc: Jan Kara, ext4 On Sun, Jan 30, 2011 at 11:44:25AM -0800, Manish Katiyar wrote: > > Do you mean separate patch for each of the changed functions ? Unless the cleanups for a set of functions are all the same issue, yes. The idea is that commit description should apply to all of the patch hunks in the commit. That way if there is a problem, it becomes easier to bisect and then determine what's going on by reading the commit description. It also becomes easier to review the changes, as well. Does that make sense? Thanks, - Ted ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-30 20:07 ` Ted Ts'o @ 2011-01-31 1:04 ` Manish Katiyar 0 siblings, 0 replies; 10+ messages in thread From: Manish Katiyar @ 2011-01-31 1:04 UTC (permalink / raw) To: Ted Ts'o; +Cc: Jan Kara, ext4 On Sun, Jan 30, 2011 at 12:07 PM, Ted Ts'o <tytso@mit.edu> wrote: > On Sun, Jan 30, 2011 at 11:44:25AM -0800, Manish Katiyar wrote: >> >> Do you mean separate patch for each of the changed functions ? > > Unless the cleanups for a set of functions are all the same issue, > yes. The idea is that commit description should apply to all of the > patch hunks in the commit. > > That way if there is a problem, it becomes easier to bisect and then > determine what's going on by reading the commit description. It also > becomes easier to review the changes, as well. > > Does that make sense? Hi Ted, Resending the above patch broken in following series of patch. -- 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] 10+ messages in thread
* Re: [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures 2011-01-25 16:17 ` Jan Kara 2011-01-25 20:05 ` Manish Katiyar @ 2011-01-30 5:29 ` Manish Katiyar 1 sibling, 0 replies; 10+ messages in thread From: Manish Katiyar @ 2011-01-30 5:29 UTC (permalink / raw) To: Jan Kara; +Cc: ext4, mkatiyar Hi Jan, Below is the updated patch based on your feedback. On Tue, Jan 25, 2011 at 8:17 AM, Jan Kara <jack@suse.cz> wrote: > Hi, > > On Sat 22-01-11 19:34:55, Manish Katiyar wrote: >> Pass GFP_KERNEL for transaction allocation for ext4 routines if caller >> can handler failures > Some error recovery paths will need cleaning up before you actually start > using them - see below: > >> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c >> index e0270d1..1a4a944 100644 >> --- a/fs/ext4/acl.c >> +++ b/fs/ext4/acl.c >> @@ -351,7 +351,7 @@ ext4_acl_chmod(struct inode *inode) >> >> retry: >> handle = ext4_journal_start(inode, >> - EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); >> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); >> if (IS_ERR(handle)) { >> error = PTR_ERR(handle); >> ext4_std_error(inode->i_sb, error); > We shouldn't call ext4_std_error() (that possibly logs the message in > the kernel log and remounts the fs read-only, panics the kernel or so) in > case of ENOMEM... > >> @@ -449,7 +449,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(inode, >> + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); >> if (IS_ERR(handle)) >> return PTR_ERR(handle); > It's actually not your bug, but the above should be: > error = PTR_ERR(handle); > goto release_and_out; > >> error = ext4_set_acl(handle, inode, type, acl); >> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c >> index eb9097a..e0e27a3 100644 >> --- a/fs/ext4/ialloc.c >> +++ b/fs/ext4/ialloc.c >> @@ -1257,7 +1257,7 @@ extern int ext4_init_inode_table(struct >> super_block *sb, ext4_group_t group, >> if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_ZEROED)) >> goto out; >> >> - handle = ext4_journal_start_sb(sb, 1); >> + handle = ext4_journal_start_sb(sb, 1, true); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> goto out; > Well, this might be disputable. This function is used to lazily > initialize inode table. If the initialization fails, thread removes the > request for initialization from the queue. But in case of ENOMEM, it might > be more suitable to just postpone the initialization work to a more > suitable time... I have converted it back to pass false. Will send a separate patch later. > >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index 9f7f9e4..76c20b8 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c > ... >> @@ -1398,7 +1398,7 @@ static int _ext4_get_block(struct inode *inode, >> sector_t iblock, >> if (map.m_len > DIO_MAX_BLOCKS) >> map.m_len = DIO_MAX_BLOCKS; >> dio_credits = ext4_chunk_trans_blocks(inode, map.m_len); >> - handle = ext4_journal_start(inode, dio_credits); >> + handle = ext4_journal_start(inode, dio_credits, false); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> return ret; > Hmm, this would be actually another useful prerequisite cleanup of this > series. _ext4_get_block() should need to start a transaction only when > called from direct IO path (otherwise transaction should be already started > when creating blocks). But this is only implicit so it would be good to > create ext4_get_block_directIO() which would start a transaction, use it > as a callback of __blockdev_direct_IO(), and remove the code from standard > _ext4_get_block() function. Then you can also make ext4_journal_start() > possibly fail and still it will be clear you do not introduce any potential > problems. > >> @@ -1621,7 +1621,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(inode, needed_blocks, false); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> goto out; > Failing this with ENOMEM is OK. Note that grab_cache_page_write_begin() > called just below can fail with ENOMEM as well. Using the same reasoning, I also changed ext4_da_write_begin(). > >> @@ -3553,7 +3554,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(inode, 2, false); >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> goto out; > This can fail without introducing problems. It's standard directIO write > path. > >> @@ -3596,7 +3597,7 @@ retry: >> int err; >> >> /* Credits for sb + inode write */ >> - handle = ext4_journal_start(inode, 2); >> + handle = ext4_journal_start(inode, 2, false); >> if (IS_ERR(handle)) { >> /* This is really bad luck. We've written the data >> * but cannot extend i_size. Bail out and pretend > This one can fail just fine as well. > >> @@ -5329,8 +5330,10 @@ 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(inode, >> + (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+ >> + EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3, >> + true); >> if (IS_ERR(handle)) { >> error = PTR_ERR(handle); >> goto err_out; >> @@ -5365,7 +5368,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(inode, 3, true); >> if (IS_ERR(handle)) { >> error = PTR_ERR(handle); >> goto err_out; > The above two sites are fine but note that err_out calls ext4_std_error() > which we don't want to happen in case of ENOMEM. > >> @@ -5822,7 +5825,7 @@ int ext4_change_inode_journal_flag(struct inode >> *inode, int val) >> >> /* Finally we can mark the inode as dirty. */ >> >> - handle = ext4_journal_start(inode, 1); >> + handle = ext4_journal_start(inode, 1, true); >> if (IS_ERR(handle)) >> return PTR_ERR(handle); > This can fail OK, but you should undo inode flag and aops change before > returning error (that would be probably better as a separate preparatory > patch because it won't be completely trivial - you need to lock the updates > again etc. possibly create a helper function for that so that you don't > duplicate the code). Changed back to pass false. Will do the cleanup and error handling as part of separate patch once I start understanding the code better. For ext4 functions which can handle transaction allocation failures, pass GFP_KERNEL flag for transaction allocation dring journal start. Otherwise GFP_NOFS is passed which retries in a loop till allocation succeeds. Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> --- fs/ext4/acl.c | 12 +++++++----- fs/ext4/ext4_jbd2.h | 9 +++++---- fs/ext4/extents.c | 8 ++++---- fs/ext4/ialloc.c | 2 +- fs/ext4/inode.c | 37 +++++++++++++++++++++---------------- fs/ext4/ioctl.c | 4 ++-- fs/ext4/migrate.c | 4 ++-- fs/ext4/move_extent.c | 2 +- fs/ext4/namei.c | 24 +++++++++++++++--------- fs/ext4/resize.c | 8 ++++---- fs/ext4/super.c | 13 +++++++------ fs/ext4/xattr.c | 3 ++- 12 files changed, 71 insertions(+), 55 deletions(-) diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index e0270d1..0f7aac2 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -351,10 +351,9 @@ ext4_acl_chmod(struct inode *inode) retry: handle = ext4_journal_start(inode, - EXT4_DATA_TRANS_BLOCKS(inode->i_sb)); + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); 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,9 +448,12 @@ 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)); - if (IS_ERR(handle)) - return PTR_ERR(handle); + handle = ext4_journal_start(inode, + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); + if (IS_ERR(handle)) { + error = PTR_ERR(handle); + goto release_and_out; + } error = ext4_set_acl(handle, inode, type, acl); ext4_journal_stop(handle); if (error == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries)) diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h index d8b992e..38f128e 100644 --- a/fs/ext4/ext4_jbd2.h +++ b/fs/ext4/ext4_jbd2.h @@ -161,7 +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(struct super_block *sb, int nblocks); +handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks, bool errok); int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle); #define EXT4_NOJOURNAL_MAX_REF_COUNT ((unsigned long) 4096) @@ -209,9 +209,10 @@ static inline void ext4_journal_release_buffer(handle_t *handle, jbd2_journal_release_buffer(handle, bh); } -static inline handle_t *ext4_journal_start(struct inode *inode, int nblocks) +static inline handle_t *ext4_journal_start(struct inode *inode, + int nblocks, bool errok) { - return ext4_journal_start_sb(inode->i_sb, nblocks); + return ext4_journal_start_sb(inode->i_sb, nblocks, errok); } #define ext4_journal_stop(handle) \ @@ -232,7 +233,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/extents.c b/fs/ext4/extents.c index 63a7581..5944b1c 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -2333,7 +2333,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start) ext_debug("truncate since %u\n", start); /* probably first extent we're gonna free will be last in block */ - handle = ext4_journal_start(inode, depth + 1); + handle = ext4_journal_start(inode, depth + 1, false); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -3544,7 +3544,7 @@ void ext4_ext_truncate(struct inode *inode) * probably first extent we're gonna free will be last in block */ err = ext4_writepage_trans_blocks(inode); - handle = ext4_journal_start(inode, err); + handle = ext4_journal_start(inode, err, false); if (IS_ERR(handle)) return; @@ -3677,7 +3677,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(inode, credits, true); if (IS_ERR(handle)) { ret = PTR_ERR(handle); break; @@ -3752,7 +3752,7 @@ int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset, while (ret >= 0 && ret < max_blocks) { map.m_lblk += ret; map.m_len = (max_blocks -= ret); - handle = ext4_journal_start(inode, credits); + handle = ext4_journal_start(inode, credits, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); break; diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index eb9097a..4499dcd 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -1257,7 +1257,7 @@ extern int ext4_init_inode_table(struct super_block *sb, ext4_group_t group, if (gdp->bg_flags & cpu_to_le16(EXT4_BG_INODE_ZEROED)) goto out; - handle = ext4_journal_start_sb(sb, 1); + handle = ext4_journal_start_sb(sb, 1, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 9f7f9e4..12a9b74 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -129,7 +129,7 @@ static handle_t *start_transaction(struct inode *inode) { handle_t *result; - result = ext4_journal_start(inode, blocks_for_truncate(inode)); + result = ext4_journal_start(inode, blocks_for_truncate(inode), false); if (!IS_ERR(result)) return result; @@ -204,7 +204,7 @@ void ext4_evict_inode(struct inode *inode) if (is_bad_inode(inode)) goto no_delete; - handle = ext4_journal_start(inode, blocks_for_truncate(inode)+3); + handle = ext4_journal_start(inode, blocks_for_truncate(inode)+3, false); if (IS_ERR(handle)) { ext4_std_error(inode->i_sb, PTR_ERR(handle)); /* @@ -1398,7 +1398,7 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock, if (map.m_len > DIO_MAX_BLOCKS) map.m_len = DIO_MAX_BLOCKS; dio_credits = ext4_chunk_trans_blocks(inode, map.m_len); - handle = ext4_journal_start(inode, dio_credits); + handle = ext4_journal_start(inode, dio_credits, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); return ret; @@ -1621,7 +1621,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(inode, needed_blocks, true); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -2653,7 +2653,8 @@ static int __ext4_journalled_writepage(struct page *page, * references to buffers so we are safe */ unlock_page(page); - handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode)); + handle = ext4_journal_start(inode, ext4_writepage_trans_blocks(inode), + false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -3049,7 +3050,7 @@ retry: needed_blocks = ext4_da_writepages_trans_blocks(inode); /* start a new transaction*/ - handle = ext4_journal_start(inode, needed_blocks); + handle = ext4_journal_start(inode, needed_blocks, false); if (IS_ERR(handle)) { ret = PTR_ERR(handle); ext4_msg(inode->i_sb, KERN_CRIT, "%s: jbd2_start: " @@ -3204,7 +3205,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(inode, 1, true); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -3553,7 +3554,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(inode, 2, true); if (IS_ERR(handle)) { ret = PTR_ERR(handle); goto out; @@ -3596,7 +3597,7 @@ retry: int err; /* Credits for sb + inode write */ - handle = ext4_journal_start(inode, 2); + handle = ext4_journal_start(inode, 2, true); if (IS_ERR(handle)) { /* This is really bad luck. We've written the data * but cannot extend i_size. Bail out and pretend @@ -5329,8 +5330,10 @@ 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(inode, + (EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb)+ + EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb))+3, + true); if (IS_ERR(handle)) { error = PTR_ERR(handle); goto err_out; @@ -5365,7 +5368,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(inode, 3, true); if (IS_ERR(handle)) { error = PTR_ERR(handle); goto err_out; @@ -5385,7 +5388,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) attr->ia_size); if (error) { /* Do as much error cleanup as possible */ - handle = ext4_journal_start(inode, 3); + handle = ext4_journal_start(inode, 3, false); if (IS_ERR(handle)) { ext4_orphan_del(NULL, inode); goto err_out; @@ -5421,7 +5424,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; @@ -5738,7 +5743,7 @@ void ext4_dirty_inode(struct inode *inode) { handle_t *handle; - handle = ext4_journal_start(inode, 2); + handle = ext4_journal_start(inode, 2, false); if (IS_ERR(handle)) goto out; @@ -5822,7 +5827,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val) /* Finally we can mark the inode as dirty. */ - handle = ext4_journal_start(inode, 1); + handle = ext4_journal_start(inode, 1, false); if (IS_ERR(handle)) return PTR_ERR(handle); diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index eb3bc2f..a357b9a 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(inode, 1, true); 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(inode, 1, true); if (IS_ERR(handle)) { err = PTR_ERR(handle); goto setversion_out; diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c index b0a126f..729dcbc 100644 --- a/fs/ext4/migrate.c +++ b/fs/ext4/migrate.c @@ -488,7 +488,7 @@ int ext4_ext_migrate(struct inode *inode) EXT4_DATA_TRANS_BLOCKS(inode->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb) - + 1); + + 1, true); if (IS_ERR(handle)) { retval = PTR_ERR(handle); return retval; @@ -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(inode, 1, true); 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..d5aad4d 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(orig_inode, jblocks, true); if (IS_ERR(handle)) { *err = PTR_ERR(handle); return 0; diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 5485390..a3ad11f 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1739,7 +1739,8 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, int mode, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -1775,7 +1776,8 @@ static int ext4_mknod(struct inode *dir, struct dentry *dentry, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -1814,7 +1816,8 @@ static int ext4_mkdir(struct inode *dir, struct dentry *dentry, int mode) retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2127,7 +2130,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(dir, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb), true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2188,7 +2192,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(dir, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb), true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2246,8 +2251,9 @@ static int ext4_symlink(struct inode *dir, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 5 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 5 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb), + true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2313,7 +2319,7 @@ static int ext4_link(struct dentry *old_dentry, retry: handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS); + EXT4_INDEX_EXTRA_TRANS_BLOCKS, true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2365,7 +2371,7 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, dquot_initialize(new_dentry->d_inode); handle = ext4_journal_start(old_dir, 2 * EXT4_DATA_TRANS_BLOCKS(old_dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2); + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2, true); if (IS_ERR(handle)) return PTR_ERR(handle); diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c index 3ecc6e4..e50d083 100644 --- a/fs/ext4/resize.c +++ b/fs/ext4/resize.c @@ -176,7 +176,7 @@ static int setup_new_group_blocks(struct super_block *sb, int err = 0, err2; /* This transaction may be extended/restarted along the way */ - handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA); + handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA, true); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -655,7 +655,7 @@ static void update_backups(struct super_block *sb, handle_t *handle; int err = 0, err2; - handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA); + handle = ext4_journal_start_sb(sb, EXT4_MAX_TRANS_DATA, true); if (IS_ERR(handle)) { group = 1; err = PTR_ERR(handle); @@ -793,7 +793,7 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input) */ handle = ext4_journal_start_sb(sb, ext4_bg_has_super(sb, input->group) ? - 3 + reserved_gdb : 4); + 3 + reserved_gdb : 4, true); if (IS_ERR(handle)) { err = PTR_ERR(handle); goto exit_put; @@ -1031,7 +1031,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es, /* We will update the superblock, one block bitmap, and * one group descriptor via ext4_free_blocks(). */ - handle = ext4_journal_start_sb(sb, 3); + handle = ext4_journal_start_sb(sb, 3, true); if (IS_ERR(handle)) { err = PTR_ERR(handle); ext4_warning(sb, "error %d on journal start", err); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index cb10a06..eacfea7 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -241,7 +241,7 @@ static void ext4_put_nojournal(handle_t *handle) * that sync() will call the filesystem's write_super callback if * appropriate. */ -handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) +handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks, bool errok) { journal_t *journal; @@ -258,7 +258,7 @@ 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); } return ext4_get_nojournal(); } @@ -4471,7 +4471,8 @@ static int ext4_write_dquot(struct dquot *dquot) inode = dquot_to_inode(dquot); handle = ext4_journal_start(inode, - EXT4_QUOTA_TRANS_BLOCKS(dquot->dq_sb)); + EXT4_QUOTA_TRANS_BLOCKS(dquot->dq_sb), + false); if (IS_ERR(handle)) return PTR_ERR(handle); ret = dquot_commit(dquot); @@ -4487,7 +4488,7 @@ static int ext4_acquire_dquot(struct dquot *dquot) handle_t *handle; handle = ext4_journal_start(dquot_to_inode(dquot), - EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb)); + EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb), false); if (IS_ERR(handle)) return PTR_ERR(handle); ret = dquot_acquire(dquot); @@ -4503,7 +4504,7 @@ static int ext4_release_dquot(struct dquot *dquot) handle_t *handle; handle = ext4_journal_start(dquot_to_inode(dquot), - EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb)); + EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb), false); if (IS_ERR(handle)) { /* Release dquot anyway to avoid endless cycle in dqput() */ dquot_release(dquot); @@ -4534,7 +4535,7 @@ static int ext4_write_info(struct super_block *sb, int type) handle_t *handle; /* Data block + inode block */ - handle = ext4_journal_start(sb->s_root->d_inode, 2); + handle = ext4_journal_start(sb->s_root->d_inode, 2, false); if (IS_ERR(handle)) return PTR_ERR(handle); ret = dquot_commit_info(sb, type); diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index fc32176..0e39f57 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(inode, + EXT4_DATA_TRANS_BLOCKS(inode->i_sb), true); if (IS_ERR(handle)) { error = PTR_ERR(handle); } else { -- 1.6.0.4 Thanks - Manish > >> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c >> index eb3bc2f..3e7977b 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(inode, 1, false); >> if (IS_ERR(handle)) { >> err = PTR_ERR(handle); >> goto flags_out; > This can handle failure just fine... > >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index cb10a06..7714a15 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -4487,7 +4488,7 @@ static int ext4_acquire_dquot(struct dquot *dquot) >> handle_t *handle; >> >> handle = ext4_journal_start(dquot_to_inode(dquot), >> - EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb)); >> + EXT4_QUOTA_INIT_BLOCKS(dquot->dq_sb), true); >> if (IS_ERR(handle)) >> return PTR_ERR(handle); >> ret = dquot_acquire(dquot); >> @@ -4503,7 +4504,7 @@ static int ext4_release_dquot(struct dquot *dquot) >> handle_t *handle; >> >> handle = ext4_journal_start(dquot_to_inode(dquot), >> - EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb)); >> + EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb), true); >> if (IS_ERR(handle)) { >> /* Release dquot anyway to avoid endless cycle in dqput() */ >> dquot_release(dquot); > For now put 'false' in these quota functions. Because failure here > results in a failure of dquot_initialize() which is not tested in most > places and thus results in quota miscomputations... Properly handling this > would require another set of cleanups. > > 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 related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-01-31 1:04 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-23 3:34 [PATCH 2/3] ext4 : Pass GFP_KERNEL for transaction allocation if caller can handler failures Manish Katiyar 2011-01-25 16:17 ` Jan Kara 2011-01-25 20:05 ` Manish Katiyar 2011-01-26 7:55 ` Lukas Czerner 2011-01-26 7:58 ` Manish Katiyar 2011-01-30 19:42 ` Ted Ts'o 2011-01-30 19:44 ` Manish Katiyar 2011-01-30 20:07 ` Ted Ts'o 2011-01-31 1:04 ` Manish Katiyar 2011-01-30 5:29 ` Manish Katiyar
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).