ocfs2: Let ocfs2_setattr use new truncate sequence.
in [Kernel]
|
Prev: Question on siig sata 3 controller
Next: [PATCH] of/device: Move struct of_device define outside of CONFIG_OF_DEVICE test
From: Nick Piggin on 10 Jun 2010 00:50 On Thu, Jun 10, 2010 at 11:53:06AM +0800, Tao Ma wrote: > Let ocfs2 use the new truncate sequence. The changes include: > 1. Remove the extra check for inode_newsize_ok since Christoph > has moved it into inode_change_ok. So we will check it at the > beginning of ocfs2_setattr. So this deals with our questions regarding check of i_size outside the inode cluster lock? (see fsdevel discussion) > 2. Use truncate_setsize directly since we don't implement our > own ->truncate and what we need is "update i_size and > truncate_pagecache" which truncate_setsize now does. > 3. For direct write, ocfs2 actually don't allow write to pass > i_size(see ocfs2_prepare_inode_for_write), so we don't have > a chance to increase i_size. So remove the bogus check. > > Cc: Joel Becker <joel.becker(a)oracle.com> > Cc: Christoph Hellwig <hch(a)lst.de> > Cc: Nick Piggin <npiggin(a)suse.de> > Signed-off-by: Tao Ma <tao.ma(a)oracle.com> > --- > fs/ocfs2/file.c | 34 +++++----------------------------- > 1 files changed, 5 insertions(+), 29 deletions(-) > > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c > index 1fb0985..764fffb 100644 > --- a/fs/ocfs2/file.c > +++ b/fs/ocfs2/file.c > @@ -983,10 +983,6 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) > } > > if (size_change && attr->ia_size != i_size_read(inode)) { > - status = inode_newsize_ok(inode, attr->ia_size); > - if (status) > - goto bail_unlock; > - > if (i_size_read(inode) > attr->ia_size) { While you're here, you should be able to use inode->i_size if you're under i_mutex, no? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tao Ma on 10 Jun 2010 01:10 Hi Nick, On 06/10/2010 12:42 PM, Nick Piggin wrote: > On Thu, Jun 10, 2010 at 11:53:06AM +0800, Tao Ma wrote: >> Let ocfs2 use the new truncate sequence. The changes include: >> 1. Remove the extra check for inode_newsize_ok since Christoph >> has moved it into inode_change_ok. So we will check it at the >> beginning of ocfs2_setattr. > > So this deals with our questions regarding check of i_size outside > the inode cluster lock? (see fsdevel discussion) oh, I forget about this. yes, we should have cluster lock and shouldn't remove this check. > > >> 2. Use truncate_setsize directly since we don't implement our >> own ->truncate and what we need is "update i_size and >> truncate_pagecache" which truncate_setsize now does. >> 3. For direct write, ocfs2 actually don't allow write to pass >> i_size(see ocfs2_prepare_inode_for_write), so we don't have >> a chance to increase i_size. So remove the bogus check. >> >> Cc: Joel Becker<joel.becker(a)oracle.com> >> Cc: Christoph Hellwig<hch(a)lst.de> >> Cc: Nick Piggin<npiggin(a)suse.de> >> Signed-off-by: Tao Ma<tao.ma(a)oracle.com> >> --- >> fs/ocfs2/file.c | 34 +++++----------------------------- >> 1 files changed, 5 insertions(+), 29 deletions(-) >> >> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >> index 1fb0985..764fffb 100644 >> --- a/fs/ocfs2/file.c >> +++ b/fs/ocfs2/file.c >> @@ -983,10 +983,6 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr) >> } >> >> if (size_change&& attr->ia_size != i_size_read(inode)) { >> - status = inode_newsize_ok(inode, attr->ia_size); >> - if (status) >> - goto bail_unlock; >> - >> if (i_size_read(inode)> attr->ia_size) { > > While you're here, you should be able to use inode->i_size if you're > under i_mutex, no? ok, will change it and the correpsonding part in truncate_setsize. Regards, Tao -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Hellwig on 10 Jun 2010 04:30 On Thu, Jun 10, 2010 at 01:08:05PM +0800, Tao Ma wrote: > Let ocfs2 use the new truncate sequence. The changes include: > 1. Use truncate_setsize directly since we don't implement our > own ->truncate and what we need is "update i_size and > truncate_pagecache" which truncate_setsize now does. > 2. For direct write, ocfs2 actually don't allow write to pass > i_size(see ocfs2_prepare_inode_for_write), so we don't have > a chance to increase i_size. So remove the bogus check. You just leave the duplicate inode_newsize_ok in, but still have one as part of inode_change_ok. See the previous thread - we'll need to move inode_change_ok to under the cluster locks, both for the truncate and non-truncate case. > /* > + * Since all the work for a size change has been done above. > + * Call truncate_setsize directly to change size and truncate > + * pagecache. > */ > if ((attr->ia_valid & ATTR_SIZE) && > + attr->ia_size != inode->i_size) this could be on one line now. > + truncate_setsize(inode, attr->ia_size); But any reason this isn't done inside the if (size_change && attr->ia_size != inode->i_size) { conditional above? You'll never get size and uid/gid changes in the same request, so there won't be any change in behaviour. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tao Ma on 10 Jun 2010 04:50 On 06/10/2010 04:27 PM, Christoph Hellwig wrote: > On Thu, Jun 10, 2010 at 01:08:05PM +0800, Tao Ma wrote: >> Let ocfs2 use the new truncate sequence. The changes include: >> 1. Use truncate_setsize directly since we don't implement our >> own ->truncate and what we need is "update i_size and >> truncate_pagecache" which truncate_setsize now does. >> 2. For direct write, ocfs2 actually don't allow write to pass >> i_size(see ocfs2_prepare_inode_for_write), so we don't have >> a chance to increase i_size. So remove the bogus check. > > You just leave the duplicate inode_newsize_ok in, but still have > one as part of inode_change_ok. See the previous thread - we'll > need to move inode_change_ok to under the cluster locks, both > for the truncate and non-truncate case. uh, I just don't change the original inode_change_ok, and maybe you are right that we should check all these under cluster lock. But it looks as if it is written like this intentionally. Mark and Joel, do you have any option that why we write like this or it is a bug? > >> /* >> + * Since all the work for a size change has been done above. >> + * Call truncate_setsize directly to change size and truncate >> + * pagecache. >> */ >> if ((attr->ia_valid& ATTR_SIZE)&& >> + attr->ia_size != inode->i_size) > > this could be on one line now. ok, I will regenerate the patch after I get the feedback from Mark and Joel. > >> + truncate_setsize(inode, attr->ia_size); > > But any reason this isn't done inside the > > if (size_change&& attr->ia_size != inode->i_size) { > > conditional above? You'll never get size and uid/gid changes in the > same request, so there won't be any change in behaviour. Because we want the inode change in a transaction. In the above condition, we do truncate/extend in a transaction. And after it is done, we start a new transaction that update the inode info. Regards, Tao -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Joel Becker on 10 Jun 2010 04:50
On Thu, Jun 10, 2010 at 10:27:11AM +0200, Christoph Hellwig wrote: > You just leave the duplicate inode_newsize_ok in, but still have > one as part of inode_change_ok. See the previous thread - we'll > need to move inode_change_ok to under the cluster locks, both > for the truncate and non-truncate case. Is your concern that the u/gid checks may be against stale ids? > > + truncate_setsize(inode, attr->ia_size); > > But any reason this isn't done inside the > > if (size_change && attr->ia_size != inode->i_size) { > > conditional above? You'll never get size and uid/gid changes in the > same request, so there won't be any change in behaviour. I think the code exists as-is so that the i_size update only happens after the quota transfer has been approved. Jan added the quota bits in this location. I can't see a standard posix op that changes size and ids at the same time. I think we just add BUG_ON expressions that ensure such a behavior, right? Joel -- "I'm living so far beyond my income that we may almost be said to be living apart." - e e cummings Joel Becker Principal Software Developer Oracle E-mail: joel.becker(a)oracle.com Phone: (650) 506-8127 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |