Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Nick Piggin]

On Wed, Jun 09, 2010 at 04:48:20PM +0800, Tao Ma wrote:
> Nick has changed the turncate sequence and now inode_setattr
> is deprecated. ocfs2 has done the most work of i_size change
> before calling inode_setattr, so we are safe to just call
> simple_setsize to update the i_size in vfs inode and then
> generic_setattr.
> 
> As for dlmfs, we don't allow inode size change, so it is safe
> for us to just call generic_setattr.
> 
> Cc: Nick Piggin <npiggin@suse.de>
> Signed-off-by: Tao Ma <tao.ma@oracle.com>
> ---
>  fs/ocfs2/dlmfs/dlmfs.c |    2 +-
>  fs/ocfs2/file.c        |   18 +++++++++---------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
> index b83d610..e959b88 100644
> --- a/fs/ocfs2/dlmfs/dlmfs.c
> +++ b/fs/ocfs2/dlmfs/dlmfs.c
> @@ -215,7 +215,7 @@ static int dlmfs_file_setattr(struct dentry *dentry, struct iattr *attr)
>  	attr->ia_valid &= ~ATTR_SIZE;
>  	error = inode_change_ok(inode, attr);
>  	if (!error)
> -		error = inode_setattr(inode, attr);
> +		generic_setattr(inode, attr);

This brings up my question again about simply _ignoring_ ATTR_SIZE changes.
One problem is that truncate calls which send down ATTR_SIZE might also
be sending down ATTR_MODE or ATTR_?TIME which should not be applied if
the ATTR_SIZE fails, one would think.

The other thing is that shouldn't we return -EINVAL or somesuch rather
than silently ignoring?

Anyway, several other pseudo filesystems do other stuff like tihs so
it's not something to bother with in this patch.


>  
>  	return error;
>  }
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 6a13ea6..06fd85c 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1052,17 +1052,17 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
>  	}
>  
>  	/*
> -	 * This will intentionally not wind up calling simple_setsize(),
> -	 * since all the work for a size change has been done above.
> -	 * Otherwise, we could get into problems with truncate as
> -	 * ip_alloc_sem is used there to protect against i_size
> -	 * changes.
> +	 * Since all the work for a size change has been done above,
> +	 * we only need to call simple_setsize to update i_size.
>  	 */
> -	status = inode_setattr(inode, attr);
> -	if (status < 0) {
> -		mlog_errno(status);
> -		goto bail_commit;
> +	if (attr->ia_valid & ATTR_SIZE) {
> +		status = simple_setsize(inode, attr->ia_size);
> +		if (status < 0) {
> +			mlog_errno(status);
> +			goto bail_commit;
> +		}
>  	}
> +	generic_setattr(inode, attr);
>  
>  	status = ocfs2_mark_inode_dirty(handle, inode, bh);
>  	if (status < 0)

simple_setsize of course calls inode_newsize_ok, which may fail. If
you're committing your on-disk truncates at this point, you will
prefer to hoist that check as early as possible so you cannot fail
here.

Just open code the truncate part of simple_setsize for now.

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Tao Ma]

Hi Nick,
	Thanks for the quick response.
On 06/09/2010 05:16 PM, Nick Piggin wrote:
> On Wed, Jun 09, 2010 at 04:48:20PM +0800, Tao Ma wrote:
>> Nick has changed the turncate sequence and now inode_setattr
>> is deprecated. ocfs2 has done the most work of i_size change
>> before calling inode_setattr, so we are safe to just call
>> simple_setsize to update the i_size in vfs inode and then
>> generic_setattr.
>>
>> As for dlmfs, we don't allow inode size change, so it is safe
>> for us to just call generic_setattr.
>>
>> Cc: Nick Piggin<npiggin@suse.de>
>> Signed-off-by: Tao Ma<tao.ma@oracle.com>
>> ---
>>   fs/ocfs2/dlmfs/dlmfs.c |    2 +-
>>   fs/ocfs2/file.c        |   18 +++++++++---------
>>   2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
>> index b83d610..e959b88 100644
>> --- a/fs/ocfs2/dlmfs/dlmfs.c
>> +++ b/fs/ocfs2/dlmfs/dlmfs.c
>> @@ -215,7 +215,7 @@ static int dlmfs_file_setattr(struct dentry *dentry, struct iattr *attr)
>>   	attr->ia_valid&= ~ATTR_SIZE;
>>   	error = inode_change_ok(inode, attr);
>>   	if (!error)
>> -		error = inode_setattr(inode, attr);
>> +		generic_setattr(inode, attr);
>
> This brings up my question again about simply _ignoring_ ATTR_SIZE changes.
> One problem is that truncate calls which send down ATTR_SIZE might also
> be sending down ATTR_MODE or ATTR_?TIME which should not be applied if
> the ATTR_SIZE fails, one would think.
>
> The other thing is that shouldn't we return -EINVAL or somesuch rather
> than silently ignoring?
>
> Anyway, several other pseudo filesystems do other stuff like tihs so
> it's not something to bother with in this patch.
yeah, it looks that we have a little chance to fail out here.
>
>
>>
>>   	return error;
>>   }
>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>> index 6a13ea6..06fd85c 100644
>> --- a/fs/ocfs2/file.c
>> +++ b/fs/ocfs2/file.c
>> @@ -1052,17 +1052,17 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
>>   	}
>>
>>   	/*
>> -	 * This will intentionally not wind up calling simple_setsize(),
>> -	 * since all the work for a size change has been done above.
>> -	 * Otherwise, we could get into problems with truncate as
>> -	 * ip_alloc_sem is used there to protect against i_size
>> -	 * changes.
>> +	 * Since all the work for a size change has been done above,
>> +	 * we only need to call simple_setsize to update i_size.
>>   	 */
>> -	status = inode_setattr(inode, attr);
>> -	if (status<  0) {
>> -		mlog_errno(status);
>> -		goto bail_commit;
>> +	if (attr->ia_valid&  ATTR_SIZE) {
>> +		status = simple_setsize(inode, attr->ia_size);
>> +		if (status<  0) {
>> +			mlog_errno(status);
>> +			goto bail_commit;
>> +		}
>>   	}
>> +	generic_setattr(inode, attr);
>>
>>   	status = ocfs2_mark_inode_dirty(handle, inode, bh);
>>   	if (status<  0)
>
> simple_setsize of course calls inode_newsize_ok, which may fail. If
> you're committing your on-disk truncates at this point, you will
> prefer to hoist that check as early as possible so you cannot fail
> here.
>
We have a inode_newsize_ok check at the top of ocfs2_setattr when we do 
the real work of truncate(it is far above, so the patch can't have it. 
;) ). So we don't need to worry about that actually.
> Just open code the truncate part of simple_setsize for now.

Regards,
Tao

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Nick Piggin]

On Wed, Jun 09, 2010 at 05:29:37PM +0800, Tao Ma wrote:
> Hi Nick,
> 	Thanks for the quick response.
> On 06/09/2010 05:16 PM, Nick Piggin wrote:
> >On Wed, Jun 09, 2010 at 04:48:20PM +0800, Tao Ma wrote:
> >>@@ -1052,17 +1052,17 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
> >>  	}
> >>
> >>  	/*
> >>-	 * This will intentionally not wind up calling simple_setsize(),
> >>-	 * since all the work for a size change has been done above.
> >>-	 * Otherwise, we could get into problems with truncate as
> >>-	 * ip_alloc_sem is used there to protect against i_size
> >>-	 * changes.
> >>+	 * Since all the work for a size change has been done above,
> >>+	 * we only need to call simple_setsize to update i_size.
> >>  	 */
> >>-	status = inode_setattr(inode, attr);
> >>-	if (status<  0) {
> >>-		mlog_errno(status);
> >>-		goto bail_commit;
> >>+	if (attr->ia_valid&  ATTR_SIZE) {
> >>+		status = simple_setsize(inode, attr->ia_size);
> >>+		if (status<  0) {
> >>+			mlog_errno(status);
> >>+			goto bail_commit;
> >>+		}
> >>  	}
> >>+	generic_setattr(inode, attr);
> >>
> >>  	status = ocfs2_mark_inode_dirty(handle, inode, bh);
> >>  	if (status<  0)
> >
> >simple_setsize of course calls inode_newsize_ok, which may fail. If
> >you're committing your on-disk truncates at this point, you will
> >prefer to hoist that check as early as possible so you cannot fail
> >here.
> >
> We have a inode_newsize_ok check at the top of ocfs2_setattr when we
> do the real work of truncate(it is far above, so the patch can't
> have it. ;) ). So we don't need to worry about that actually.

OK, can you comment that or just open-code the truncate part of
simple_setsize? It should be helpful for example with Christoph's
truncate cleanup work.

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Joel Becker]

On Wed, Jun 09, 2010 at 07:38:04PM +1000, Nick Piggin wrote:
> On Wed, Jun 09, 2010 at 05:29:37PM +0800, Tao Ma wrote:
> > We have a inode_newsize_ok check at the top of ocfs2_setattr when we
> > do the real work of truncate(it is far above, so the patch can't
> > have it. ;) ). So we don't need to worry about that actually.
> 
> OK, can you comment that or just open-code the truncate part of
> simple_setsize? It should be helpful for example with Christoph's
> truncate cleanup work.

	Just open-code it.  The void calls are much nicer than useless
return code boilerplate. 

Joel

-- 

 One look at the From:
 understanding has blossomed
 .procmailrc grows
	- Alexander Viro

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Christoph Hellwig]

On Wed, Jun 09, 2010 at 03:12:07AM -0700, Joel Becker wrote:
> On Wed, Jun 09, 2010 at 07:38:04PM +1000, Nick Piggin wrote:
> > On Wed, Jun 09, 2010 at 05:29:37PM +0800, Tao Ma wrote:
> > > We have a inode_newsize_ok check at the top of ocfs2_setattr when we
> > > do the real work of truncate(it is far above, so the patch can't
> > > have it. ;) ). So we don't need to worry about that actually.
> > 
> > OK, can you comment that or just open-code the truncate part of
> > simple_setsize? It should be helpful for example with Christoph's
> > truncate cleanup work.
> 
> 	Just open-code it.  The void calls are much nicer than useless
> return code boilerplate. 

Note that if you rebase against vfs.git #for_next simple_setsize is
replaced with a truncate_setsize that does all the work but the
inode_newsize_ok check.  As I mentioned before please do the work ontop
of that branch, it does make the ->setattr ATTR_SIZE implementation much
nicer for the filesystems.

Re: [Ocfs2-devel] [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Joel Becker]

On Wed, Jun 09, 2010 at 12:14:34PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 09, 2010 at 03:12:07AM -0700, Joel Becker wrote:
> > > OK, can you comment that or just open-code the truncate part of
> > > simple_setsize? It should be helpful for example with Christoph's
> > > truncate cleanup work.
> > 
> > 	Just open-code it.  The void calls are much nicer than useless
> > return code boilerplate. 
> 
> Note that if you rebase against vfs.git #for_next simple_setsize is
> replaced with a truncate_setsize that does all the work but the
> inode_newsize_ok check.  As I mentioned before please do the work ontop
> of that branch, it does make the ->setattr ATTR_SIZE implementation much
> nicer for the filesystems.

	Works for me.

Joel

-- 

"Glory is fleeting, but obscurity is forever."  
         - Napoleon Bonaparte

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Christoph Hellwig]

Tao, can you rebase this in Al's vfs.git for-next queue (or just
linux-next which includes it)

There's lots of changes to the calling sequences in that area in there
and having all conversions based ontop of that would make life a lot
simpler for everyone.

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Tao Ma]

Christoph Hellwig wrote:
> Tao, can you rebase this in Al's vfs.git for-next queue (or just
> linux-next which includes it)
>
> There's lots of changes to the calling sequences in that area in there
> and having all conversions based ontop of that would make life a lot
> simpler for everyone.
>   
ok, so it looks I am too eager to merge the new truncate sequence into 
ocfs2. ;)

Regards,
Tao

Re: [PATCH] ocfs2: Remove the deprecated inode_setattr.

[Nick Piggin]

On Wed, Jun 09, 2010 at 03:08:39AM -0700, Joel Becker wrote:
> On Wed, Jun 09, 2010 at 04:48:20PM +0800, Tao Ma wrote:
> > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> > index 6a13ea6..06fd85c 100644
> > --- a/fs/ocfs2/file.c
> > +++ b/fs/ocfs2/file.c
> > @@ -1052,17 +1052,17 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
> >  	}
> >  
> >  	/*
> > -	 * This will intentionally not wind up calling simple_setsize(),
> > -	 * since all the work for a size change has been done above.
> > -	 * Otherwise, we could get into problems with truncate as
> > -	 * ip_alloc_sem is used there to protect against i_size
> > -	 * changes.
> > +	 * Since all the work for a size change has been done above,
> > +	 * we only need to call simple_setsize to update i_size.
> >  	 */
> > -	status = inode_setattr(inode, attr);
> > -	if (status < 0) {
> > -		mlog_errno(status);
> > -		goto bail_commit;
> > +	if (attr->ia_valid & ATTR_SIZE) {
> > +		status = simple_setsize(inode, attr->ia_size);
> > +		if (status < 0) {
> > +			mlog_errno(status);
> > +			goto bail_commit;
> > +		}
> 
> 	Boy, this is a bit to think about.  The old code deliberately
> used vmtruncate() to update i_size and trim the pagecace, but because we
> didn't implement ->truncate() we knew that we wouldn't modify the
> (already adjusted) allocation.  Not buggy, Christoph, just a roundabout
> way to get the job done.

Good to have that confirmed.


> 	The new simple_setsize() does nothing but update i_size and trim
> the pagecache, right Nick?  If that's so, that's all we need.

Yep, check out vmtruncate:

int vmtruncate(struct inode *inode, loff_t offset)
{
        int error;

        error = simple_setsize(inode, offset);
        if (error)
                return error;

        if (inode->i_op->truncate)
                inode->i_op->truncate(inode);

        return error;
}

Of course, simple_setsize has the inode_newsize_ok check, which you
should be calling before you even start adjusting your allocation.
So please open-code the pagecache truncation part of this, or better
yet if you push the change through Al's tree, use Christoph's new
truncate_pagecache helper.

Also, I hope you're all following the setattr/truncate discussions on
fsdevel? setattr is in a bit of a mess in a lot of fs; it will be nice
to know exactly when various inode attributes are valid to test, to
start with.