You can therefore fool it by doing something like this:
bash-2.05b# /sbin/sysctl -w pvfs2.acache.timeout-msecs=100000000
bash-2.05b# mkdir -p /mnt/pvfs2/a/b/c
bash-2.05b# rm -rf /mnt/pvfs2/a
rm: cannot remove directory `/mnt/pvfs2/a/b': Directory not empty
The cache timeout doesn't have to be that high to show the
problem. We should be able to stay consistent on a single client
regardless of the cache setting at any rate anyway.
I don't think that removing the check will really hurt much:
- the directory entry has already been removed at this point in the
operation, so it doesn't do anything to help avoid cleanup scenarios
- the server will perform this check on its own when it gets the
remove request
So having the check might have made remove _failures_ faster (since
a client potentially reach the decision that a directory wasn't
empty earlier) but it doesn't change the success path, and doesn't
alter semantics or race condition behavior.
pvfs2-rename-dirent-count.patch:
=======================
This fixes a related but different bug in rename, in the tricky
case in which someone tries to rename a directory to another
directory, but the destination directory is not empty:
bash-2.05b# /sbin/sysctl -w pvfs2.acache.timeout-msecs=10000000
bash-2.05b# mkdir -p /mnt/pvfs2/scratch/boom
bash-2.05b# mkdir -p /mnt/pvfs2/scratch/yeah/1
bash-2.05b# rename boom yeah /mnt/pvfs2/scratch/boom
rename: renaming /mnt/pvfs2/scratch/boom to /mnt/pvfs2/scratch/yeah
failed: Directory not empty
bash-2.05b# ls /mnt/pvfs2/scratch/yeah
I hate that the rename system call even lets you rename to an
existing file/directory name. Trying to modify multiple points in
the name space somewhat atomically isn't fun. At any rate there
were two problems:
- We are doing a dirent_count check on the client side, but not
asking for the dirent_count in the getattr mask. It therefore
usually showed up as zero.
- The same caching problem as described in the remove patch
theoretically applies here, but in this case we really need to be
able to do this check on the client side. I say theoretical
because I couldn't come up with a test case to triger it after
making the above mask fix, but I am still nervous about it. This
fix here was to add a flag to the getattr nested machine to force
it to bypass the cache in cases (this one!) where we want the
getattr information to be as up to date as possible. The flag
could be set by any state machine that uses the nested getattr
machine in case we ever find another use for it.
The fundamental problem with renaming directories to existing names
is that up to 4 metadata servers may be involved when you consider
the two directories and the two parent directories. There is no
way to get decent consistency with the client driving the operation
in this special case unless you retrieve the dirent_count and check
it on the client.
-Phil
---------------------
PatchSet 378
Date: 2006/01/16 17:34:18
Author: pcarns
Branch: HEAD
Tag: (none)
Log:
bug fix: make sure that we request the dirent_count attribute when
performing a getattr on the destination directory during rename; it
is needed
for a safety check in the following state.
[artf29248]
Members:
src/client/sysint/sys-rename.sm:1.9->1.10
Index: src/client/sysint/sys-rename.sm
diff -u src/client/sysint/sys-rename.sm:1.9 src/client/sysint/sys-
rename.sm:1.10
--- src/client/sysint/sys-rename.sm:1.9 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-rename.sm Mon Jan 16 10:34:18 2006
@@ -773,6 +773,8 @@
lld(attr->u.dir.dirent_count));
gossip_debug(GOSSIP_CLIENT_DEBUG,
"rename dest attr mask: %d\n", (int)attr->mask);
+ gossip_debug(GOSSIP_CLIENT_DEBUG,
+ "rename dest handle %llu\n", llu(sm_p-
>object_ref.handle));
}
/* if the destination is a directory, is it empty? */
@@ -1006,9 +1008,13 @@
sm_p->object_ref = sm_p->u.rename.refns[1];
PINT_SM_GETATTR_STATE_CLEAR(sm_p->getattr);
+ /* NOTE: we ask for the dirent count on the destination. This is
+ * important so that we can confirm that the destination is
empty if it
+ * happens to be a directory rather than a file.
+ */
PINT_SM_GETATTR_STATE_FILL(sm_p->getattr,
sm_p->object_ref,
- PVFS_ATTR_COMMON_ALL,
+ (PVFS_ATTR_COMMON_ALL|
PVFS_ATTR_DIR_DIRENT_COUNT),
PVFS_TYPE_NONE);
js_p->error_code = 0;
return(1);
---------------------
PatchSet 379
Date: 2006/01/16 18:06:44
Author: pcarns
Branch: HEAD
Tag: (none)
Log:
Added flag parameter to nested getattr state machine, only
supported flag
right now is BYPASS_CACHE, which is useful in rename to make sure
that we get
a relatively up to date dirent_count before modifying entries
[artf29248]
Members:
src/client/sysint/client-state-machine.h:1.17->1.18
src/client/sysint/mgmt-get-dfile-array.sm:1.5->1.6
src/client/sysint/remove.sm:1.6->1.7
src/client/sysint/sys-create.sm:1.14->1.15
src/client/sysint/sys-flush.sm:1.6->1.7
src/client/sysint/sys-getattr.sm:1.11->1.12
src/client/sysint/sys-io.sm:1.18->1.19
src/client/sysint/sys-lookup.sm:1.4->1.5
src/client/sysint/sys-mkdir.sm:1.10->1.11
src/client/sysint/sys-readdir.sm:1.6->1.7
src/client/sysint/sys-rename.sm:1.10->1.11
src/client/sysint/sys-symlink.sm:1.10->1.11
src/client/sysint/sys-truncate.sm:1.8->1.9
Index: src/client/sysint/client-state-machine.h
diff -u src/client/sysint/client-state-machine.h:1.17 src/client/
sysint/client-state-machine.h:1.18
--- src/client/sysint/client-state-machine.h:1.17 Mon Jan 9
10:12:32 2006
+++ src/client/sysint/client-state-machine.h Mon Jan 16 11:06:44 2006
@@ -344,6 +344,9 @@
int persist_config_buffers;
};
+/* flag to disable cached lookup during getattr nested sm */
+#define PINT_SM_GETATTR_BYPASS_CACHE 1
+
typedef struct PINT_sm_getattr_state
{
PVFS_object_ref object_ref;
@@ -363,16 +366,19 @@
PVFS_size * size_array;
PVFS_size size;
+
+ int flags;
} PINT_sm_getattr_state;
-#define PINT_SM_GETATTR_STATE_FILL(_state, _objref, _mask,
_reftype) \
+#define PINT_SM_GETATTR_STATE_FILL(_state, _objref, _mask,
_reftype, _flags) \
do { \
memset(&(_state), 0, sizeof(PINT_sm_getattr_state)); \
(_state).object_ref.fs_id = (_objref).fs_id; \
(_state).object_ref.handle = (_objref).handle; \
(_state).req_attrmask = _mask; \
(_state).ref_type = _reftype; \
+ (_state).flags = _flags; \
} while(0)
#define PINT_SM_GETATTR_STATE_CLEAR(_state) \
Index: src/client/sysint/mgmt-get-dfile-array.sm
diff -u src/client/sysint/mgmt-get-dfile-array.sm:1.5 src/client/
sysint/mgmt-get-dfile-array.sm:1.6
--- src/client/sysint/mgmt-get-dfile-array.sm:1.5 Tue Oct 25
13:52:45 2005
+++ src/client/sysint/mgmt-get-dfile-array.sm Mon Jan 16 11:06:44 2006
@@ -91,7 +91,8 @@
sm_p->getattr,
ref,
PVFS_ATTR_META_ALL|PVFS_ATTR_COMMON_TYPE,
- PVFS_TYPE_METAFILE);
+ PVFS_TYPE_METAFILE,
+ 0);
return PINT_client_state_machine_post(
sm_p, PVFS_MGMT_GET_DFILE_ARRAY, op_id, user_ptr);
Index: src/client/sysint/remove.sm
diff -u src/client/sysint/remove.sm:1.6 src/client/sysint/remove.sm:
1.7
--- src/client/sysint/remove.sm:1.6 Mon Jan 16 09:26:36 2006
+++ src/client/sysint/remove.sm Mon Jan 16 11:06:44 2006
@@ -145,7 +145,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_META_ALL|PVFS_ATTR_COMMON_TYPE,
- PVFS_TYPE_NONE);
+ PVFS_TYPE_NONE,
+ 0);
return 1;
}
Index: src/client/sysint/sys-create.sm
diff -u src/client/sysint/sys-create.sm:1.14 src/client/sysint/sys-
create.sm:1.15
--- src/client/sysint/sys-create.sm:1.14 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-create.sm Mon Jan 16 11:06:44 2006
@@ -421,7 +421,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_COMMON_ALL,
- PVFS_TYPE_DIRECTORY);
+ PVFS_TYPE_DIRECTORY,
+ 0);
return 1;
}
Index: src/client/sysint/sys-flush.sm
diff -u src/client/sysint/sys-flush.sm:1.6 src/client/sysint/sys-
flush.sm:1.7
--- src/client/sysint/sys-flush.sm:1.6 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-flush.sm Mon Jan 16 11:06:44 2006
@@ -119,7 +119,8 @@
sm_p->getattr,
ref,
PVFS_ATTR_META_ALL|PVFS_ATTR_COMMON_TYPE,
- PVFS_TYPE_METAFILE);
+ PVFS_TYPE_METAFILE,
+ 0);
return PINT_client_state_machine_post(
sm_p, PVFS_SYS_FLUSH, op_id, user_ptr);
Index: src/client/sysint/sys-getattr.sm
diff -u src/client/sysint/sys-getattr.sm:1.11 src/client/sysint/sys-
getattr.sm:1.12
--- src/client/sysint/sys-getattr.sm:1.11 Wed Jan 4 08:50:58 2006
+++ src/client/sysint/sys-getattr.sm Mon Jan 16 11:06:44 2006
@@ -238,7 +238,8 @@
ref,
PVFS_util_sys_to_object_attr_mask(
attrmask),
- PVFS_TYPE_NONE);
+ PVFS_TYPE_NONE,
+ 0);
return PINT_client_state_machine_post(
sm_p, PVFS_SYS_GETATTR, op_id, user_ptr);
@@ -342,6 +343,15 @@
sm_p->getattr.req_attrmask |= PVFS_ATTR_META_ALL;
}
+ if(sm_p->getattr.flags & PINT_SM_GETATTR_BYPASS_CACHE)
+ {
+ gossip_debug(GOSSIP_ACACHE_DEBUG, "acache: forced acache
miss: "
+ " [%llu]\n",
+ llu(object_ref.handle));
+ js_p->error_code = GETATTR_ACACHE_MISS;
+ return 1;
+ }
+
ret = PINT_acache_get_cached_entry(object_ref,
&sm_p->getattr.attr,
&attr_status,
Index: src/client/sysint/sys-io.sm
diff -u src/client/sysint/sys-io.sm:1.18 src/client/sysint/sys-
io.sm:1.19
--- src/client/sysint/sys-io.sm:1.18 Thu Jan 12 08:21:59 2006
+++ src/client/sysint/sys-io.sm Mon Jan 16 11:06:44 2006
@@ -387,7 +387,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_META_ALL|PVFS_ATTR_COMMON_TYPE,
- PVFS_TYPE_METAFILE);
+ PVFS_TYPE_METAFILE,
+ 0);
if (js_p->error_code == IO_RETRY)
{
Index: src/client/sysint/sys-lookup.sm
diff -u src/client/sysint/sys-lookup.sm:1.4 src/client/sysint/sys-
lookup.sm:1.5
--- src/client/sysint/sys-lookup.sm:1.4 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-lookup.sm Mon Jan 16 11:06:44 2006
@@ -709,7 +709,8 @@
sm_p->getattr,
cur_seg->seg_resolved_refn,
(PVFS_ATTR_COMMON_ALL | PVFS_ATTR_SYMLNK_ALL),
- PVFS_TYPE_NONE);
+ PVFS_TYPE_NONE,
+ 0);
js_p->error_code = 1;
}
Index: src/client/sysint/sys-mkdir.sm
diff -u src/client/sysint/sys-mkdir.sm:1.10 src/client/sysint/sys-
mkdir.sm:1.11
--- src/client/sysint/sys-mkdir.sm:1.10 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-mkdir.sm Mon Jan 16 11:06:44 2006
@@ -278,7 +278,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_COMMON_ALL,
- PVFS_TYPE_DIRECTORY);
+ PVFS_TYPE_DIRECTORY,
+ 0);
return 1;
}
Index: src/client/sysint/sys-readdir.sm
diff -u src/client/sysint/sys-readdir.sm:1.6 src/client/sysint/sys-
readdir.sm:1.7
--- src/client/sysint/sys-readdir.sm:1.6 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-readdir.sm Mon Jan 16 11:06:44 2006
@@ -199,7 +199,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_DIR_ALL,
- PVFS_TYPE_DIRECTORY);
+ PVFS_TYPE_DIRECTORY,
+ 0);
assert(js_p->error_code == 0);
Index: src/client/sysint/sys-rename.sm
diff -u src/client/sysint/sys-rename.sm:1.10 src/client/sysint/sys-
rename.sm:1.11
--- src/client/sysint/sys-rename.sm:1.10 Mon Jan 16 10:34:18 2006
+++ src/client/sysint/sys-rename.sm Mon Jan 16 11:06:44 2006
@@ -468,7 +468,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_COMMON_ALL,
- PVFS_TYPE_NONE);
+ PVFS_TYPE_NONE,
+ 0);
/* if both lookups succeeded, then we need chdirent */
return RENAME_CHDIRENT;
@@ -1015,7 +1016,8 @@
PINT_SM_GETATTR_STATE_FILL(sm_p->getattr,
sm_p->object_ref,
(PVFS_ATTR_COMMON_ALL|
PVFS_ATTR_DIR_DIRENT_COUNT),
- PVFS_TYPE_NONE);
+ PVFS_TYPE_NONE,
+ PINT_SM_GETATTR_BYPASS_CACHE);
js_p->error_code = 0;
return(1);
}
Index: src/client/sysint/sys-symlink.sm
diff -u src/client/sysint/sys-symlink.sm:1.10 src/client/sysint/sys-
symlink.sm:1.11
--- src/client/sysint/sys-symlink.sm:1.10 Tue Dec 6 13:46:30 2005
+++ src/client/sysint/sys-symlink.sm Mon Jan 16 11:06:44 2006
@@ -301,7 +301,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_COMMON_ALL,
- PVFS_TYPE_DIRECTORY);
+ PVFS_TYPE_DIRECTORY,
+ 0);
return ret;
}
Index: src/client/sysint/sys-truncate.sm
diff -u src/client/sysint/sys-truncate.sm:1.8 src/client/sysint/sys-
truncate.sm:1.9
--- src/client/sysint/sys-truncate.sm:1.8 Wed Jan 4 08:50:58 2006
+++ src/client/sysint/sys-truncate.sm Mon Jan 16 11:06:44 2006
@@ -124,7 +124,8 @@
sm_p->getattr,
sm_p->object_ref,
PVFS_ATTR_META_ALL|PVFS_ATTR_COMMON_TYPE,
- PVFS_TYPE_METAFILE);
+ PVFS_TYPE_METAFILE,
+ 0);
return PINT_client_state_machine_post(
sm_p, PVFS_SYS_TRUNCATE, op_id, user_ptr);
---------------------
PatchSet 377
Date: 2006/01/16 16:26:36
Author: pcarns
Branch: HEAD
Tag: (none)
Log:
bug fix: removed client side dirent_count check because the client's
attribute information may be too out of date to rely on
[artf29238]
Members:
src/client/sysint/remove.sm:1.5->1.6
Index: src/client/sysint/remove.sm
diff -u src/client/sysint/remove.sm:1.5 src/client/sysint/remove.sm:
1.6
--- src/client/sysint/remove.sm:1.5 Wed Jan 4 08:50:58 2006
+++ src/client/sysint/remove.sm Mon Jan 16 09:26:36 2006
@@ -170,11 +170,17 @@
js_p->error_code = REMOVE_MUST_REMOVE_DATAFILES;
break;
case PVFS_TYPE_DIRECTORY:
+#if 0
+/* NOTE: this check is not safe because it relies on cached
attributes on the
+ * parent, which may be out of date. The server will perform this
check
+ * locally when we attempt to remove the directory itself.
+ */
if(attr->u.dir.dirent_count != 0)
{
js_p->error_code = -PVFS_ENOTEMPTY;
break;
}
+#endif
case PVFS_TYPE_SYMLINK:
js_p->error_code = 0;
break;
_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
