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: Tao Ma on 10 Jun 2010 08:20 Joel Becker wrote: > 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? > So I think we should have one inode_change_ok before the cluster lock and another after the cluster lock. The first one will save us a lot of cluster lock effort if the user pass us the wrong arguments while the later one will test again with the refreshed inode info. Regards, Tao > >>> + 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 > > -- 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: Nick Piggin on 10 Jun 2010 08:30 On Thu, Jun 10, 2010 at 08:09:36PM +0800, Tao Ma wrote: > Joel Becker wrote: > >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? > So I think we should have one inode_change_ok before the cluster > lock and another after the cluster lock. > The first one will save us a lot of cluster lock effort if the user > pass us the wrong arguments while the later > one will test again with the refreshed inode info. If attributes cannot be stale, then do the checks before the cluster lock and not again. If they can be stale, then the check outside the cluster lock might give incorrect results so it is not harmless to do it twice. If you have a mix of some attributes may be stale, then why not do the inode_change_ok check inside the lock, and then do some open code checks for optimization before taking the lock. -- 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 14:20 On Thu, Jun 10, 2010 at 08:09:36PM +0800, Tao Ma wrote: > Joel Becker wrote: > > Is your concern that the u/gid checks may be against stale ids? > So I think we should have one inode_change_ok before the cluster > lock and another after the cluster lock. > The first one will save us a lot of cluster lock effort if the user > pass us the wrong arguments while the later > one will test again with the refreshed inode info. But what if the other node has given us permission, and then we fail? Say the file was owned by you. On node 2, root sets it to be owned by me. Then on node 1, I go to change the file permissions. inode_change_ok() will fail, because the in-memory inode still thinks you are the owner. I guess it does need to be under the lock. Joel -- Bram's Law: The easier a piece of software is to write, the worse it's implemented in practice. 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/
From: Tao Ma on 10 Jun 2010 20:10 Joel Becker wrote: > On Thu, Jun 10, 2010 at 08:09:36PM +0800, Tao Ma wrote: > >> Joel Becker wrote: >> >>> Is your concern that the u/gid checks may be against stale ids? >>> >> So I think we should have one inode_change_ok before the cluster >> lock and another after the cluster lock. >> The first one will save us a lot of cluster lock effort if the user >> pass us the wrong arguments while the later >> one will test again with the refreshed inode info. >> > > But what if the other node has given us permission, and then we > fail? Say the file was owned by you. On node 2, root sets it to be > owned by me. Then on node 1, I go to change the file permissions. > inode_change_ok() will fail, because the in-memory inode still thinks > you are the owner. > I guess it does need to be under the lock. > OK, so I will revise my patch to move it under cluster lock. 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: Nick Piggin on 11 Jun 2010 01:00
On Fri, Jun 11, 2010 at 11:27:49AM +0800, Tao Ma wrote: > Let ocfs2 use the new truncate sequence. The changes include: > 1. Move inode_change_ok into cluster lock and remove inode_newsize_ok. > 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. Change some i_size_read to inode->i_size in ocfs2_setattr > since we have i_muext held. > 4. 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> The patch looks fine to me. I really appreciate you working on truncate, thanks. -- 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/ |