ocfs2: Remove the deprecated inode_setattr.

PATCH In review Tao Ma 4 participants 12 messages Jun 9, 2010

Subsystems (2)

VFS OCFS2

lore

Share & tools

lore

Tao Ma

author

2010-06-09 08:48 UTC #0 lore raw

PATCH 1/1 ocfs2: Remove the deprecated inode_setattr.

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 <[email protected]>

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/dlmfs/dlmfs.c |    2 +-
 fs/ocfs2/file.c        |   18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/ocfs2/dlmfs/dlmfs.c +1 -1

index b83d610..e959b88 100644
@@ -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);
 
 	return error;
 }

fs/ocfs2/file.c +9 -9

index 6a13ea6..06fd85c 100644
@@ -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)

Signature · 1 line

1.5.5

Nick Piggin

2010-06-09 09:16 UTC #1 lore raw

Quoted 21 lines · Tao Ma

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 <[email protected]>
Signed-off-by Tao Ma <[email protected]>
---
 fs/ocfs2/dlmfs/dlmfs.c |    2 +-
 fs/ocfs2/file.c        |   18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)
fs/ocfs2/dlmfs/dlmfs.c +0 -0
index b83d610..e959b88 100644

fs/ocfs2/dlmfs/dlmfs.c static int dlmfs_file_setattr(struct dentry *dentry, struct iattr *attr) · line 218

Quoted 5 lines

fs/ocfs2/dlmfs/dlmfs.c +1 -1

@@ -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.

Quoted 24 lines

fs/ocfs2/dlmfs/dlmfs.c +0 -0

 	return error;
 }

fs/ocfs2/file.c +3 -9

index 6a13ea6..06fd85c 100644
@@ -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) {

fs/ocfs2/file.c int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1068

Quoted 10 lines

fs/ocfs2/file.c +6 -0

+		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.

Tao Ma

author

2010-06-09 09:29 UTC #2 lore raw

Hi Nick,
Thanks for the quick response.

Quoted 33 lines · Nick Piggin

Quoted 21 lines · Tao Ma
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<[email protected]>
Signed-off-by Tao Ma<[email protected]>
---
  fs/ocfs2/dlmfs/dlmfs.c |    2 +-
  fs/ocfs2/file.c        |   18 +++++++++---------
  2 files changed, 10 insertions(+), 10 deletions(-)
fs/ocfs2/dlmfs/dlmfs.c +0 -0
index b83d610..e959b88 100644
fs/ocfs2/dlmfs/dlmfs.c static int dlmfs_file_setattr(struct dentry *dentry, struct iattr *attr) · line 218
Quoted 5 lines
fs/ocfs2/dlmfs/dlmfs.c +1 -1
@@ -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.

Quoted 5 lines

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.

Quoted 34 lines

fs/ocfs2/dlmfs/dlmfs.c +0 -0

  	return error;
  }

fs/ocfs2/file.c +9 -9

index 6a13ea6..06fd85c 100644
@@ -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);

Quoted 8 lines

fs/ocfs2/file.c int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1068
Quoted 2 lines
fs/ocfs2/file.c +0 -0
  	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.

Quoted 1 line

Just open code the truncate part of simple_setsize for now.

Regards,
Tao

Nick Piggin

2010-06-09 09:38 UTC #3 lore raw

Quoted 37 lines · Tao Ma

Hi Nick,
Thanks for the quick response.

Quoted 26 lines · Nick Piggin

Quoted 25 lines · Tao Ma

fs/ocfs2/file.c +9 -9

@@ -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);

Quoted 8 lines

Review int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1068
Quoted 2 lines
(quoted diff) +0 -0
 	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.

Quoted 3 lines

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.

Joel Becker

2010-06-09 10:12 UTC #4 lore raw

Quoted 8 lines · Nick Piggin

Quoted 3 lines · Tao Ma
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

Signature · 10 lines

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

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

Christoph Hellwig

2010-06-09 10:14 UTC #5 lore raw

Quoted 6 lines · Joel Becker

Quoted 5 lines · Nick Piggin
Quoted 3 lines · Tao Ma
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.

Quoted 6 lines

Quoted 3 lines
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.

Joel Becker

2010-06-09 19:53 UTC #6 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Joel Becker
Quoted 3 lines
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.

Quoted 8 lines

Quoted 2 lines
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

Signature · 8 lines

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

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

Christoph Hellwig

2010-06-09 10:01 UTC #7 lore raw

In reply to Nick Piggin

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.

Tao Ma

author

2010-06-09 13:43 UTC #8 lore raw

Quoted 7 lines · 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.

ok, so it looks I am too eager to merge the new truncate sequence into ocfs2. ;)

Regards,
Tao

Joel Becker

2010-06-09 10:08 UTC #9 lore raw

PATCH 1/1 ocfs2: Remove the deprecated inode_setattr.

In reply to Tao Ma

Quoted 16 lines · Tao Ma

fs/ocfs2/file.c +2 -5

index 6a13ea6..06fd85c 100644
@@ -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.
 	 */

fs/ocfs2/file.c int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1063

Quoted 10 lines

fs/ocfs2/file.c +6 -4

-	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.
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.

Joel

Signature · 9 lines

"Also, all of life's big problems include the words 'indictment' or
	'inoperable.' Everything else is small stuff."
	- Alton Brown

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

Nick Piggin

2010-06-09 10:43 UTC #10 lore raw

Quoted 25 lines · Joel Becker

Quoted 24 lines · Tao Ma

fs/ocfs2/file.c +6 -9

index 6a13ea6..06fd85c 100644
@@ -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);

Quoted 8 lines

fs/ocfs2/file.c int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1063
Quoted 2 lines
fs/ocfs2/file.c +2 -0
+			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.

Quoted 2 lines

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.

Tao Ma

author

2010-06-09 13:54 UTC #11 lore raw

PATCH 1/1 ocfs2: Remove the deprecated inode_setattr.

In reply to Joel Becker

Quoted 30 lines · Joel Becker

Quoted 17 lines · Tao Ma

fs/ocfs2/file.c +2 -6

index 6a13ea6..06fd85c 100644
@@ -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);

fs/ocfs2/file.c int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1064

Quoted 10 lines

fs/ocfs2/file.c +6 -3

-	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;
+		}

Quoted 8 lines

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.
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.

yeah, the new simple_setsize did what you said.

Regards,
Tao