ocfs2: Remove the deprecated inode_setattr.
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.
--- 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
@@ -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
@@ -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
Quoted 21 lines · Tao Ma
Nick has changed the turncate sequence and now
inode_setattris deprecated. ocfs2 has done the most work ofi_sizechange before callinginode_setattr, so we are safe to just callsimple_setsizeto update thei_sizein vfs inode and thengeneric_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
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
-0return error; }fs/ocfs2/file.c +3
-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 ) {
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.
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_setattris deprecated. ocfs2 has done the most work ofi_sizechange before callinginode_setattr, so we are safe to just callsimple_setsizeto update thei_sizein vfs inode and thengeneric_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
-0fs/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_SIZEchanges. One problem is that truncate calls which send downATTR_SIZEmight also be sending downATTR_MODEor ATTR_?TIME which should not be applied if theATTR_SIZEfails, one would think.
Quoted 5 lines
The other thing is that shouldn't we return
-EINVALor 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
Quoted 34 lines
fs/ocfs2/dlmfs/dlmfs.c +0
-0return error; }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
fs/ocfs2/file.c int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) · line 1068
Quoted 2 lines
fs/ocfs2/file.c +0
-0status =ocfs2_mark_inode_dirty (handle, inode, bh);if (status<0 )
simple_setsizeof course callsinode_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_setsizefor now.
Regards,
Tao
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
-0status = ocfs2_mark_inode_dirty(handle, inode, bh); if (status< 0)
simple_setsizeof course callsinode_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_okcheck at the top ofocfs2_setattrwhen 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.
Quoted 8 lines · Nick Piggin
Quoted 3 lines · Tao Ma
We have a
inode_newsize_okcheck at the top ofocfs2_setattrwhen 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
Quoted 6 lines · Joel Becker
Quoted 5 lines · Nick Piggin
Quoted 3 lines · Tao Ma
We have a
inode_newsize_okcheck at the top ofocfs2_setattrwhen 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.
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_setsizeis replaced with atruncate_setsizethat does all the work but theinode_newsize_okcheck. As I mentioned before please do the work ontop of that branch, it does make the ->setattrATTR_SIZEimplementation 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-8127Tao, 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.
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
PATCH 1/1 ocfs2: Remove the deprecated inode_setattr.
In reply to Tao MaQuoted 16 lines · Tao Ma
fs/ocfs2/file.c +2
-5@@ -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
Quoted 25 lines · Joel Becker
Quoted 24 lines · Tao Ma
fs/ocfs2/file.c +6
-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);
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
usedvmtruncate()to updatei_sizeand 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 updatei_sizeand 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.
PATCH 1/1 ocfs2: Remove the deprecated inode_setattr.
In reply to Joel BeckerQuoted 30 lines · Joel Becker
Quoted 17 lines · Tao Ma
fs/ocfs2/file.c +2
-6@@ -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
usedvmtruncate()to updatei_sizeand 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 newsimple_setsize()does nothing but updatei_sizeand 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