[PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[Greg Kurz]

The 9p filesystem has some serious issues with all syscalls that deal
with file attributes out of a file descriptor instead of a path name
(aka. fstat, ftruncate, fchmod and friends). If the file is opened and
then unlinked, any subsequent call to the above syscalls will fail with
EACCES. The same will happen to ftruncate() if the file is no longer
writable.

The root cause to this is that the VFS layer does not pass the struct
file to the filesystem when it is about file attributes. This is
legitimate: when we reach setattr/getattr, we don't need anything to
be dereferenced from the struct file. But the current lookup code is
based on the list of fids hanging off the dentry: it is okay for path
name based syscalls, but not relialable in the case of file descriptors
because we cannot find an open fid this way.

Eric sent a patch last year to address the case when the file gets
unlinked:

https://patchwork.kernel.org/patch/6252761/

The general idea is to try to find an open fid, starting from the inode.
The patch does this by browsing the 9p client list of all fids when we
know the file was unlinked: if we find a fid, it is necessarily an open
fid.

I could find a newer version of this patch with some minor changes, here:

https://github.com/ericvh/linux/commits/v9fs-devel

Since the patch also fixes a few other things, I re-post it first in
this series. Eric, I hope it is okay for you ?

As suggested in the changelog, the lookup of open fid can be optimized
if we link open fids to the inode. This is addressed by the second patch.

The third patch addresses a related issue with ftruncate() on unwriteable
files.

This series can be tested with a custom QEMU, available here:

https://github.com/gkurz/qemu/commits/9p-attr-fixes

I could have most f*() syscalls working in the guest, with the notable
exception of xattr related ones because more code is needed in QEMU.

Please comment. Test reports will also be appreciated :)

---

Eric Van Hensbergen (1):
      fs/9p: fix create-unlink-getattr idiom

Greg Kurz (2):
      fs/9p: track open fids
      fs/9p: search open fids first


 fs/9p/fid.c             |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 fs/9p/fid.h             |    1 +
 fs/9p/vfs_dir.c         |    5 +++++
 fs/9p/vfs_file.c        |    1 +
 fs/9p/vfs_inode.c       |   10 +++++++++-
 fs/9p/vfs_inode_dotl.c  |    1 +
 include/net/9p/client.h |    3 +++
 net/9p/client.c         |    5 ++++-
 8 files changed, 69 insertions(+), 3 deletions(-)

--
Greg

[PATCH 1/3] fs/9p: fix create-unlink-getattr idiom

[Greg Kurz]

From: Eric Van Hensbergen <ericvh@gmail.com>

Fixes several outstanding bug reports of not being able to getattr from an
open file after an unlink.  This patch cleans up transient fids on an unlink
and will search open fids on a client if it detects a dentry that appears to
have been unlinked.  This search is necessary because fstat does not pass fd
information through the VFS API to the filesystem, only the dentry which for
9p has an imperfect match to fids.

Inherent in this patch is also a fix for the qid handling on create/open
which apparently wasn't being set correctly and was necessary for the search
to succeed.

A possible optimization over this fix is to include accounting of open fids
with the inode in the private data (in a similar fashion to the way we track
transient fids with dentries).  This would allow a much quicker search for
a matching open fid.

Signed-off-by: Eric Van Hensbergen <ericvh@gmail.com>
(changed v9fs_fid_find_global to v9fs_fid_find_inode in comment)
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 fs/9p/fid.c       |   30 ++++++++++++++++++++++++++++++
 fs/9p/vfs_inode.c |    4 ++++
 net/9p/client.c   |    5 ++++-
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/fs/9p/fid.c b/fs/9p/fid.c
index 47db55aee7f2..e6f81f327407 100644
--- a/fs/9p/fid.c
+++ b/fs/9p/fid.c
@@ -54,6 +54,33 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid)
 }
 
 /**
+ * v9fs_fid_find_inode - search for a fid off of the client list
+ * @inode: return a fid pointing to a specific inode
+ * @uid: return a fid belonging to the specified user
+ *
+ */
+
+static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid)
+{
+	struct p9_client *clnt = v9fs_inode2v9ses(inode)->clnt;
+	struct p9_fid *fid, *fidptr, *ret = NULL;
+	unsigned long flags;
+
+	p9_debug(P9_DEBUG_VFS, " inode: %p\n", inode);
+
+	spin_lock_irqsave(&clnt->lock, flags);
+	list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) {
+		if (uid_eq(fid->uid, uid) &&
+		   (inode->i_ino == v9fs_qid2ino(&fid->qid))) {
+			ret = fid;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&clnt->lock, flags);
+	return ret;
+}
+
+/**
  * v9fs_fid_find - retrieve a fid that belongs to the specified uid
  * @dentry: dentry to look for fid in
  * @uid: return fid that belongs to the specified user
@@ -80,6 +107,9 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any)
 			}
 		}
 		spin_unlock(&dentry->d_lock);
+	} else {
+		if (dentry->d_inode)
+			ret = v9fs_fid_find_inode(dentry->d_inode, uid);
 	}
 
 	return ret;
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index f4645c515262..729144ad0c2c 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -624,6 +624,10 @@ static int v9fs_remove(struct inode *dir, struct dentry *dentry, int flags)
 
 		v9fs_invalidate_inode_attr(inode);
 		v9fs_invalidate_inode_attr(dir);
+
+		/* invalidate all fids associated with dentry */
+		/* NOTE: This will not include open fids */
+		dentry->d_op->d_release(dentry);
 	}
 	return retval;
 }
diff --git a/net/9p/client.c b/net/9p/client.c
index 3fc94a49ccd5..98980bacefd4 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1208,7 +1208,7 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname,
 	if (nwname)
 		memmove(&fid->qid, &wqids[nwqids - 1], sizeof(struct p9_qid));
 	else
-		fid->qid = oldfid->qid;
+		memmove(&fid->qid, &oldfid->qid, sizeof(struct p9_qid));
 
 	kfree(wqids);
 	return fid;
@@ -1261,6 +1261,7 @@ int p9_client_open(struct p9_fid *fid, int mode)
 		p9_is_proto_dotl(clnt) ? "RLOPEN" : "ROPEN",  qid.type,
 		(unsigned long long)qid.path, qid.version, iounit);
 
+	memmove(&fid->qid, &qid, sizeof(struct p9_qid));
 	fid->mode = mode;
 	fid->iounit = iounit;
 
@@ -1306,6 +1307,7 @@ int p9_client_create_dotl(struct p9_fid *ofid, char *name, u32 flags, u32 mode,
 			(unsigned long long)qid->path,
 			qid->version, iounit);
 
+	memmove(&ofid->qid, qid, sizeof(struct p9_qid));
 	ofid->mode = mode;
 	ofid->iounit = iounit;
 
@@ -1351,6 +1353,7 @@ int p9_client_fcreate(struct p9_fid *fid, char *name, u32 perm, int mode,
 				(unsigned long long)qid.path,
 				qid.version, iounit);
 
+	memmove(&fid->qid, &qid, sizeof(struct p9_qid));
 	fid->mode = mode;
 	fid->iounit = iounit;
 

[PATCH 2/3] fs/9p: track open fids

[Greg Kurz]

From: Greg Kurz <gkurz@linux.vnet.ibm.com>

This patch adds accounting of open fids in a list hanging off the i_private
field of the corresponding inode. This allows faster lookups compared to
searching the full 9p client list.

The lookup code is modified accordingly.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 fs/9p/fid.c             |   31 ++++++++++++++++++++++---------
 fs/9p/fid.h             |    1 +
 fs/9p/vfs_dir.c         |    5 +++++
 fs/9p/vfs_file.c        |    1 +
 fs/9p/vfs_inode.c       |    6 +++++-
 fs/9p/vfs_inode_dotl.c  |    1 +
 include/net/9p/client.h |    3 +++
 7 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/fs/9p/fid.c b/fs/9p/fid.c
index e6f81f327407..6ac68df50dca 100644
--- a/fs/9p/fid.c
+++ b/fs/9p/fid.c
@@ -54,7 +54,7 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid)
 }
 
 /**
- * v9fs_fid_find_inode - search for a fid off of the client list
+ * v9fs_fid_find_inode - search for an open fid off of the inode list
  * @inode: return a fid pointing to a specific inode
  * @uid: return a fid belonging to the specified user
  *
@@ -62,25 +62,38 @@ void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid)
 
 static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid)
 {
-	struct p9_client *clnt = v9fs_inode2v9ses(inode)->clnt;
-	struct p9_fid *fid, *fidptr, *ret = NULL;
-	unsigned long flags;
+	struct hlist_head *h;
+	struct p9_fid *fid, *ret = NULL;
 
 	p9_debug(P9_DEBUG_VFS, " inode: %p\n", inode);
 
-	spin_lock_irqsave(&clnt->lock, flags);
-	list_for_each_entry_safe(fid, fidptr, &clnt->fidlist, flist) {
-		if (uid_eq(fid->uid, uid) &&
-		   (inode->i_ino == v9fs_qid2ino(&fid->qid))) {
+	spin_lock(&inode->i_lock);
+	h = (struct hlist_head *)&inode->i_private;
+	hlist_for_each_entry(fid, h, ilist) {
+		if (uid_eq(fid->uid, uid)) {
 			ret = fid;
 			break;
 		}
 	}
-	spin_unlock_irqrestore(&clnt->lock, flags);
+	spin_unlock(&inode->i_lock);
 	return ret;
 }
 
 /**
+ * v9fs_open_fid_add - add an open fid to an inode
+ * @dentry: inode that the fid is being added to
+ * @fid: fid to add
+ *
+ */
+
+void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid)
+{
+	spin_lock(&inode->i_lock);
+	hlist_add_head(&fid->ilist, (struct hlist_head *)&inode->i_private);
+	spin_unlock(&inode->i_lock);
+}
+
+/**
  * v9fs_fid_find - retrieve a fid that belongs to the specified uid
  * @dentry: dentry to look for fid in
  * @uid: return fid that belongs to the specified user
diff --git a/fs/9p/fid.h b/fs/9p/fid.h
index 2b6787fcb626..1042bfa06332 100644
--- a/fs/9p/fid.h
+++ b/fs/9p/fid.h
@@ -27,4 +27,5 @@ struct p9_fid *v9fs_fid_lookup(struct dentry *dentry);
 struct p9_fid *v9fs_fid_clone(struct dentry *dentry);
 void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid);
 struct p9_fid *v9fs_writeback_fid(struct dentry *dentry);
+void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid);
 #endif
diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
index b0405d6aac85..6de4f0734cc4 100644
--- a/fs/9p/vfs_dir.c
+++ b/fs/9p/vfs_dir.c
@@ -238,6 +238,11 @@ int v9fs_dir_release(struct inode *inode, struct file *filp)
 	fid = filp->private_data;
 	p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n",
 		 inode, filp, fid ? fid->fid : -1);
+
+	spin_lock(&inode->i_lock);
+	hlist_del(&fid->ilist);
+	spin_unlock(&inode->i_lock);
+
 	if (fid)
 		p9_client_clunk(fid);
 	return 0;
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index b84c291ba1eb..db85afdc476a 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -111,6 +111,7 @@ int v9fs_file_open(struct inode *inode, struct file *file)
 	mutex_unlock(&v9inode->v_mutex);
 	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
 		v9fs_cache_inode_set_cookie(inode, file);
+	v9fs_open_fid_add(inode, fid);
 	return 0;
 out_error:
 	p9_client_clunk(file->private_data);
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 729144ad0c2c..2fb4e7a9d9bb 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -278,6 +278,7 @@ int v9fs_init_inode(struct v9fs_session_info *v9ses,
 	inode->i_rdev = rdev;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 	inode->i_mapping->a_ops = &v9fs_addr_operations;
+	inode->i_private = NULL;
 
 	switch (mode & S_IFMT) {
 	case S_IFIFO:
@@ -856,6 +857,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	struct v9fs_session_info *v9ses;
 	struct p9_fid *fid, *inode_fid;
 	struct dentry *res = NULL;
+	struct inode *inode;
 
 	if (d_unhashed(dentry)) {
 		res = v9fs_vfs_lookup(dir, dentry, 0);
@@ -884,7 +886,8 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	}
 
 	v9fs_invalidate_inode_attr(dir);
-	v9inode = V9FS_I(d_inode(dentry));
+	inode = d_inode(dentry);
+	v9inode = V9FS_I(inode);
 	mutex_lock(&v9inode->v_mutex);
 	if ((v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) &&
 	    !v9inode->writeback_fid &&
@@ -912,6 +915,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	file->private_data = fid;
 	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
 		v9fs_cache_inode_set_cookie(d_inode(dentry), file);
+	v9fs_open_fid_add(inode, fid);
 
 	*opened |= FILE_CREATED;
 out:
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index a34702c998f5..2d7618b388db 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -355,6 +355,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
 	file->private_data = ofid;
 	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
 		v9fs_cache_inode_set_cookie(inode, file);
+	v9fs_open_fid_add(inode, ofid);
 	*opened |= FILE_CREATED;
 out:
 	v9fs_put_acl(dacl, pacl);
diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index c6b97e58cf84..3ee46a6e32ba 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -194,6 +194,9 @@ struct p9_fid {
 
 	struct list_head flist;
 	struct hlist_node dlist;	/* list of all fids attached to a dentry */
+	struct hlist_node ilist;	/* list of all open fids attached to an
+					 * inode
+					 */
 };
 
 /**

[PATCH 3/3] fs/9p: search open fids first

[Greg Kurz]

A previous patch fixed the "create-unlink-getattr" idiom: if getattr is
called on an unlinked file, we try to find an open fid attached to the
corresponding inode.

We have a similar issue with file permissions and setattr:

open("./test.txt", O_RDWR|O_CREAT, 0666) = 4
chmod("./test.txt", 0)                  = 0
truncate("./test.txt", 0)               = -1 EACCES (Permission denied)
ftruncate(4, 0)                         = -1 EACCES (Permission denied)

The failure is expected with truncate() but not with ftruncate().

This happens because the lookup code does find a matching fid in the
dentry list. Unfortunately, this is not an open fid and the server
will be forced to rely on the path name, rather than on an open file
descriptor. This is the case in QEMU: the setattr operation will use
truncate() and fail because of bad write permissions.

This patch changes the logic in the lookup code, so that we consider
open fids first. It gives a chance to the server to match this open
fid to an open file descriptor and use ftruncate() instead of truncate().
This does not change the current behaviour for truncate() and other
path name based syscalls, since file permissions are checked earlier
in the VFS layer.

With this patch, we get:

open("./test.txt", O_RDWR|O_CREAT, 0666) = 4
chmod("./test.txt", 0)                  = 0
truncate("./test.txt", 0)               = -1 EACCES (Permission denied)
ftruncate(4, 0)                         = 0

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fs/9p/fid.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/9p/fid.c b/fs/9p/fid.c
index 6ac68df50dca..ffe945995378 100644
--- a/fs/9p/fid.c
+++ b/fs/9p/fid.c
@@ -109,8 +109,12 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any)
 		 dentry, dentry, from_kuid(&init_user_ns, uid),
 		 any);
 	ret = NULL;
+
+	if (d_inode(dentry))
+		ret = v9fs_fid_find_inode(d_inode(dentry), uid);
+
 	/* we'll recheck under lock if there's anything to look in */
-	if (dentry->d_fsdata) {
+	if (!ret && dentry->d_fsdata) {
 		struct hlist_head *h = (struct hlist_head *)&dentry->d_fsdata;
 		spin_lock(&dentry->d_lock);
 		hlist_for_each_entry(fid, h, dlist) {
@@ -120,9 +124,6 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any)
 			}
 		}
 		spin_unlock(&dentry->d_lock);
-	} else {
-		if (dentry->d_inode)
-			ret = v9fs_fid_find_inode(dentry->d_inode, uid);
 	}
 
 	return ret;

Re: [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[Dominique Martinet]

I *think* this introduces a race somewhere, I'm getting errors like:
cat: f.05: No such file or directory
cat: f.14: No such file or directory
cat: f.13: No such file or directory
cat: f.39: No such file or directory
cat: f.05: No such file or directory


when doing:
   for file in {01..50}; do touch f.${file}; done
   seq 1 1000 | xargs -n 1 -P 25 -I{} cat f.* > /dev/null



I don't get it everytime but close enough to. Server is bi-socket and
has some numa effects which help producing data-synchronization races,
it's probably harder to hit on a laptop.

I'm simply trying over a patched qemu for now, applied patches right on
top of 4.6.1, can't seem to reproduce with a vanilla 4.6.1 without any
change to qemu (still patched), so it looks kernel-side.


Can't say I've taken much time to look at the patches yet though,
sorry - I don't think it's too hard to debug though so I'll take a look
tomorrow as soon as I find time if you haven't gotten it by then.

-- 
Dominique

Re: [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[Greg Kurz]

On Mon, 4 Jul 2016 16:16:55 +0200
Dominique Martinet <dominique.martinet@cea.fr> wrote:

> I *think* this introduces a race somewhere, I'm getting errors like:
> cat: f.05: No such file or directory
> cat: f.14: No such file or directory
> cat: f.13: No such file or directory
> cat: f.39: No such file or directory
> cat: f.05: No such file or directory
> 
> 
> when doing:
>    for file in {01..50}; do touch f.${file}; done
>    seq 1 1000 | xargs -n 1 -P 25 -I{} cat f.* > /dev/null
> 
> 
> 
> I don't get it everytime but close enough to. Server is bi-socket and
> has some numa effects which help producing data-synchronization races,
> it's probably harder to hit on a laptop.
> 
> I'm simply trying over a patched qemu for now, applied patches right on
> top of 4.6.1, can't seem to reproduce with a vanilla 4.6.1 without any
> change to qemu (still patched), so it looks kernel-side.
> 
> 
> Can't say I've taken much time to look at the patches yet though,
> sorry - I don't think it's too hard to debug though so I'll take a look
> tomorrow as soon as I find time if you haven't gotten it by then.
> 

Hi Dominique !

Thanks a lot for your testing. I'll try to reproduce on my POWER8 system.

Cheers.

--
Greg

Re: [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[Dominique Martinet]

Hi Greg,

Greg Kurz wrote on Mon, Jul 04, 2016 at 05:08:49PM +0200:
> On Mon, 4 Jul 2016 16:16:55 +0200
> Dominique Martinet <dominique.martinet@cea.fr> wrote:
> 
> > I *think* this introduces a race somewhere, I'm getting errors like:
> > cat: f.05: No such file or directory
> > cat: f.14: No such file or directory
> > cat: f.13: No such file or directory
> > cat: f.39: No such file or directory
> > cat: f.05: No such file or directory
> > 
> > 
> > when doing:
> >    for file in {01..50}; do touch f.${file}; done
> >    seq 1 1000 | xargs -n 1 -P 25 -I{} cat f.* > /dev/null

Ok so, tested with the first two patches and I can't seem to hit any
problem with the qemu server at least (I'd need more time to fix
ganesha's 9p tcp/rdma server before I could blame the client in any way)


The last patch looks good to me, I think it only makes an existing race
more visible... What I think could happen is:
 process 1 has file open
 process 2 tries to open file, sees fid open
 process 1 closes file/clunk fids
 process 2 tries to clone now-clunked fid and gets ENOENT


I'm afraid I just found out my hypervisor is no longer recent enough for
gdb kernel scripts (gdb 7.6 and python 2.7.5 in el7 compared to the
apparently required 7.7 and 2.7.6 respectively...), and I don't see
anything obvious with just debug messages/adding a few printks (wasn't
able to confirm where exactly that ENOENT comes from or if my theory is
even close to the truth)

I'd like to spend more time on it but don't think I'll be able to for a
couple of weeks ; sorry about that.


Were you able to reproduce the problem?

Thanks,
-- 
Dominique

Re: [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[Greg Kurz]

On Thu, 7 Jul 2016 14:35:40 +0200
Dominique Martinet <dominique.martinet@cea.fr> wrote:

> Hi Greg,
> 

Hi Dominique,

> Greg Kurz wrote on Mon, Jul 04, 2016 at 05:08:49PM +0200:
> > On Mon, 4 Jul 2016 16:16:55 +0200
> > Dominique Martinet <dominique.martinet@cea.fr> wrote:
> >   
> > > I *think* this introduces a race somewhere, I'm getting errors like:
> > > cat: f.05: No such file or directory
> > > cat: f.14: No such file or directory
> > > cat: f.13: No such file or directory
> > > cat: f.39: No such file or directory
> > > cat: f.05: No such file or directory
> > > 
> > > 
> > > when doing:
> > >    for file in {01..50}; do touch f.${file}; done
> > >    seq 1 1000 | xargs -n 1 -P 25 -I{} cat f.* > /dev/null  
> 
> Ok so, tested with the first two patches and I can't seem to hit any
> problem with the qemu server at least (I'd need more time to fix
> ganesha's 9p tcp/rdma server before I could blame the client in any way)
> 

I'm not surprised: patch 1 simply adds a "fallback" lookup to the existing code,
and patch 2 changes this "fallback" lookup only.

Bad things can come with patch 3 because it really changes the lookup logic.

> 
> The last patch looks good to me, I think it only makes an existing race
> more visible... What I think could happen is:
>  process 1 has file open
>  process 2 tries to open file, sees fid open
>  process 1 closes file/clunk fids
>  process 2 tries to clone now-clunked fid and gets ENOENT
> 

I'll try to have a look with this scenario in mind.

> 
> I'm afraid I just found out my hypervisor is no longer recent enough for
> gdb kernel scripts (gdb 7.6 and python 2.7.5 in el7 compared to the
> apparently required 7.7 and 2.7.6 respectively...), and I don't see
> anything obvious with just debug messages/adding a few printks (wasn't
> able to confirm where exactly that ENOENT comes from or if my theory is
> even close to the truth)
> 
> I'd like to spend more time on it but don't think I'll be able to for a
> couple of weeks ; sorry about that.
> 

No problem. My plate is full anyway until I go into a 1-month vacation,
starting end of July. And I'm currently targeting QEMU 2.8 for the
server side fixes: we have plenty of time to fix this.

> 
> Were you able to reproduce the problem?
> 

Yes ! I get it every time :)

> Thanks,

I really appreciate your assistance since v9fs-devel is really quiet these
days.

Cheers.

--
Greg

Re: [PATCH 0/3] fs/9p: fix setattr/getattr issues with open files

[Greg Kurz]

On Thu, 7 Jul 2016 15:34:34 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 7 Jul 2016 14:35:40 +0200
> Dominique Martinet <dominique.martinet@cea.fr> wrote:
> 
> > Hi Greg,
> >   
> 
> Hi Dominique,
> 
> > Greg Kurz wrote on Mon, Jul 04, 2016 at 05:08:49PM +0200:  
> > > On Mon, 4 Jul 2016 16:16:55 +0200
> > > Dominique Martinet <dominique.martinet@cea.fr> wrote:
> > >     
> > > > I *think* this introduces a race somewhere, I'm getting errors like:
> > > > cat: f.05: No such file or directory
> > > > cat: f.14: No such file or directory
> > > > cat: f.13: No such file or directory
> > > > cat: f.39: No such file or directory
> > > > cat: f.05: No such file or directory
> > > > 
> > > > 
> > > > when doing:
> > > >    for file in {01..50}; do touch f.${file}; done
> > > >    seq 1 1000 | xargs -n 1 -P 25 -I{} cat f.* > /dev/null    
> > 
> > Ok so, tested with the first two patches and I can't seem to hit any
> > problem with the qemu server at least (I'd need more time to fix
> > ganesha's 9p tcp/rdma server before I could blame the client in any way)
> >   
> 
> I'm not surprised: patch 1 simply adds a "fallback" lookup to the existing code,
> and patch 2 changes this "fallback" lookup only.
> 
> Bad things can come with patch 3 because it really changes the lookup logic.
> 
> > 
> > The last patch looks good to me, I think it only makes an existing race
> > more visible... What I think could happen is:
> >  process 1 has file open
> >  process 2 tries to open file, sees fid open
> >  process 1 closes file/clunk fids
> >  process 2 tries to clone now-clunked fid and gets ENOENT
> >   
> 
> I'll try to have a look with this scenario in mind.
> 

The error indeed comes from v9fs_file_open()->v9fs_fid_clone(). I'll
try to find a fix next week.

Cheers.

--
Greg


> > 
> > I'm afraid I just found out my hypervisor is no longer recent enough for
> > gdb kernel scripts (gdb 7.6 and python 2.7.5 in el7 compared to the
> > apparently required 7.7 and 2.7.6 respectively...), and I don't see
> > anything obvious with just debug messages/adding a few printks (wasn't
> > able to confirm where exactly that ENOENT comes from or if my theory is
> > even close to the truth)
> > 
> > I'd like to spend more time on it but don't think I'll be able to for a
> > couple of weeks ; sorry about that.
> >   
> 
> No problem. My plate is full anyway until I go into a 1-month vacation,
> starting end of July. And I'm currently targeting QEMU 2.8 for the
> server side fixes: we have plenty of time to fix this.
> 
> > 
> > Were you able to reproduce the problem?
> >   
> 
> Yes ! I get it every time :)
> 
> > Thanks,  
> 
> I really appreciate your assistance since v9fs-devel is really quiet these
> days.
> 
> Cheers.
> 
> --
> Greg