Re: [PATCH 2/5] ext4 : Update low level ext4 journal routines to specify gfp_mask for transaction allocation.

[Jan Kara <jack@suse.cz>]

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