[V9fs-developer] fs/9p: fix create-unlink-getattr idiom
diff mbox

Message ID CAFkjPT=RW9z7-Dsometjzo2JPTKrVN+5dcmtM8y3viTuas4mgQ@mail.gmail.com
State Rejected
Headers show

Commit Message

Eric Van Hensbergen April 21, 2015, 7:56 p.m. UTC
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>
---
 fs/9p/fid.c            | 30 ++++++++++++++++++++++++++++++
 fs/9p/vfs_inode.c      |  4 ++++
 fs/9p/vfs_inode_dotl.c | 11 +++++++----
 net/9p/client.c        |  6 ++++--
 4 files changed, 45 insertions(+), 6 deletions(-)

  return fid;
@@ -1266,6 +1266,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;

@@ -1311,6 +1312,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;

@@ -1356,9 +1358,9 @@ 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;
-
 free_and_error:
  p9_free_req(clnt, req);
 error:

Comments

Dominique Martinet April 22, 2015, 8 a.m. UTC | #1
Eric Van Hensbergen wrote on Tue, Apr 21, 2015 at 02:56:10PM -0500:
> 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.
> 

For what it's worth, this doesn't seem to fix stat-after-unlink for me?
I'll try to look why the find doesn't work when I can.
Tested with rdma/nfs-ganesha and virtio/qemu

Quick tests prove alot more stable than the previous patch though, I'll
repost if I stump on something.

> @@ -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);

Is there a way to check if the file is open at all when we do this?
(could be we don't have dentry->d_inode if it's not, I don't know)

> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 6054c16b..ab9c49e 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -475,7 +475,7 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct
> dentry *dentry,
>   struct kstat *stat)
>  {
>   struct v9fs_session_info *v9ses;
> - struct p9_fid *fid;
> + struct p9_fid *fid = NULL;
>   struct p9_stat_dotl *st;
> 
>   p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
> @@ -484,9 +484,12 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt,
> struct dentry *dentry,
>   generic_fillattr(dentry->d_inode, stat);
>   return 0;
>   }
> - fid = v9fs_fid_lookup(dentry);
> - if (IS_ERR(fid))
> - return PTR_ERR(fid);
> +
> + if (fid == NULL) {
> + fid = v9fs_fid_lookup(dentry);
> + if (IS_ERR(fid))
> + return PTR_ERR(fid);
> + }

fid can't be anything but NULL here - think you tried to do something
else and forgot to cleanup


> - fid->qid = oldfid->qid;
> + memmove(&fid->qid, &oldfid->qid, sizeof(struct p9_qid));

good catch on all of these...
Eric Van Hensbergen April 22, 2015, 3:13 p.m. UTC | #2
On Wed, Apr 22, 2015 at 3:00 AM, Dominique Martinet
<dominique.martinet@cea.fr> wrote:
> Eric Van Hensbergen wrote on Tue, Apr 21, 2015 at 02:56:10PM -0500:
>> 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.
>>
>
> For what it's worth, this doesn't seem to fix stat-after-unlink for me?
> I'll try to look why the find doesn't work when I can.
> Tested with rdma/nfs-ganesha and virtio/qemu
>

There are changes required in the server-side as well.  I was testing with
a modification to kvmtool, I haven't looked at what was necessary to
propagate to virtio/qemu or nfs-ganesha.  At the very least it should now
be using the open fid for the stat.  The server needs to recognize that
and use fstat instead of lstat.

> Quick tests prove alot more stable than the previous patch though, I'll
> repost if I stump on something.
>

That's good at least.   Let me know.

>> @@ -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);
>
> Is there a way to check if the file is open at all when we do this?
> (could be we don't have dentry->d_inode if it's not, I don't know)
>

Not sure if that is accounted for anywhere in a reference count --
I'd imagine it would have to be in the inode versus the dentry.  Its
possible we could try and keep such a reference count to keep things
efficinet.  I believe we have a container_of() style wrapper on the inode
for caches that we could add such a count to.  Of course, if we switch
to hanging open fids off of the inode structure, it will provide a reasonably
quick method (no fids, nothing open) -- and I do think that's the right thing
to do, I'll do one more spin on this patch by the end of the upcoming
weekend with the open-fids hung off the inode structure.

>> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
>> index 6054c16b..ab9c49e 100644
>> --- a/fs/9p/vfs_inode_dotl.c
>> +++ b/fs/9p/vfs_inode_dotl.c
>> @@ -475,7 +475,7 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct
>> dentry *dentry,
>>   struct kstat *stat)
>>  {
>>   struct v9fs_session_info *v9ses;
>> - struct p9_fid *fid;
>> + struct p9_fid *fid = NULL;
>>   struct p9_stat_dotl *st;
>>
>>   p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
>> @@ -484,9 +484,12 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt,
>> struct dentry *dentry,
>>   generic_fillattr(dentry->d_inode, stat);
>>   return 0;
>>   }
>> - fid = v9fs_fid_lookup(dentry);
>> - if (IS_ERR(fid))
>> - return PTR_ERR(fid);
>> +
>> + if (fid == NULL) {
>> + fid = v9fs_fid_lookup(dentry);
>> + if (IS_ERR(fid))
>> + return PTR_ERR(fid);
>> + }
>
> fid can't be anything but NULL here - think you tried to do something
> else and forgot to cleanup
>

Good catch, yeah that was from a previous iteration on things where I was
calling the search specifically from gettattr instead of piggybacking it on
the generic fid search.  I'll clean this up.

           -eric

------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
Develop your own process in accordance with the BPMN 2 standard
Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF

Patch
diff mbox

diff --git a/fs/9p/fid.c b/fs/9p/fid.c
index 47db55a..5ca6e67 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_global - 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 3662f1d..7405149 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -625,6 +625,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/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 6054c16b..ab9c49e 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -475,7 +475,7 @@  v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct
dentry *dentry,
  struct kstat *stat)
 {
  struct v9fs_session_info *v9ses;
- struct p9_fid *fid;
+ struct p9_fid *fid = NULL;
  struct p9_stat_dotl *st;

  p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
@@ -484,9 +484,12 @@  v9fs_vfs_getattr_dotl(struct vfsmount *mnt,
struct dentry *dentry,
  generic_fillattr(dentry->d_inode, stat);
  return 0;
  }
- fid = v9fs_fid_lookup(dentry);
- if (IS_ERR(fid))
- return PTR_ERR(fid);
+
+ if (fid == NULL) {
+ fid = v9fs_fid_lookup(dentry);
+ if (IS_ERR(fid))
+ return PTR_ERR(fid);
+ }

  /* Ask for all the fields in stat structure. Server will return
  * whatever it supports
diff --git a/net/9p/client.c b/net/9p/client.c
index e86a9be..2ff06dd 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1213,7 +1213,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);