On Wed, Jan 30, 2013 at 02:42:14PM +0000, Al Viro wrote:
> On Tue, Jan 29, 2013 at 01:03:23PM -0800, Sage Weil wrote:
>
> > We should drop teh mds_client.c hunk from your patch, and then do
> > something like the below. I'll put it in the ceph tree so we can do some
> > basic testing. Unfortunately, the abort case is a bit annoying to
> > trigger.. we'll need to write a new test for it.
>
> hunk dropped, the rest folded into your patch, resulting branch pushed...
BTW, moderately related issue - ceph_get_dentry_parent_inode() uses
look fishy. In ceph_rename() it seems to be pointless. We do have
the directory inode of parent of old_dentry - it's old_dir, so it's just
a fancy way to spell igrab(old_directory). And as far as I can tell, *all*
other callers are racy.
* link("a/foo", "b/bar") can happen in parallel with
rename("a/foo", "c/splat"). link(2) holds ->i_mutex on a/foo (so it can't race
with unlink) and on b/; rename(2) holds it on a/, c/ and c/splat (if the last
one exists). It also holds ->s_vfs_rename_sem, so cross-directory renames
are serialized. For normal filesystems that's enough and there ->link()
couldn't care less about a/; ceph_link() wants the inode of a/ for some
reason. If it *really* needs the parent of a/foo for the operation, the
current code is SOL - the directory we grab can cease being that parent
just as it's getting returned to ceph_link()
* ->d_revalidate() can happen in parallel with rename(). You
don't seem to be using the parent inode much in there, so that should
be reasonably easy to deal with.
* ceph_open() can race with rename(); ->atomic_open() is
called with parent locked, but ->open() isn't. AFAICS, open() with
O_TRUNC could step into that code...
* ceph_setattr() *definitely* can race with rename(); we have
the object itself locked, but not its parent (and I'm really surprised
by the need to know that parent for such operation).
* CEPH_IOC_SET_LAYOUT vs. rename() - no idea what that ioctl is
about, but opened files can be moved around, TYVM, even when they are
in the middle of ioctl(2).
* ->setxattr() and ->removexattr() - again, can happen in parallel
with rename(), and again I really wonder why do we need the parent directory
for such operations.
What's going on there?
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
