| Message ID | 1556201060-7947-9-git-send-email-bfields@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exposing knfsd opens to userspace | expand |
On Thu, 2019-04-25 at 10:04 -0400, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > Add a nfsd/clients/#/opens file to list some information about all the > opens held by the given client. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/nfs4state.c | 100 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 98 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 928705fc8ff5..829d1e5440d3 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -684,7 +684,8 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla > > idr_preload(GFP_KERNEL); > spin_lock(&cl->cl_lock); > - new_id = idr_alloc_cyclic(&cl->cl_stateids, stid, 0, 0, GFP_NOWAIT); > + /* Reserving 0 for start of file in nfsdfs "opens" file: */ > + new_id = idr_alloc_cyclic(&cl->cl_stateids, stid, 1, 0, GFP_NOWAIT); > spin_unlock(&cl->cl_lock); > idr_preload_end(); > if (new_id < 0) > @@ -2223,9 +2224,104 @@ static const struct file_operations client_info_fops = { > .release = single_release, > }; > > +static void *opens_start(struct seq_file *s, loff_t *pos) > + __acquires(&clp->cl_lock) > +{ > + struct nfs4_client *clp = s->private; > + unsigned long id = *pos; > + void *ret; > + > + spin_lock(&clp->cl_lock); > + ret = idr_get_next_ul(&clp->cl_stateids, &id); > + *pos = id; > + return ret; > +} > + > +static void *opens_next(struct seq_file *s, void *v, loff_t *pos) > +{ > + struct nfs4_client *clp = s->private; > + unsigned long id = *pos; > + void *ret; > + > + id = *pos; > + id++; > + ret = idr_get_next_ul(&clp->cl_stateids, &id); > + *pos = id; > + return ret; > +} > + > +static void opens_stop(struct seq_file *s, void *v) > + __releases(&clp->cl_lock) > +{ > + struct nfs4_client *clp = s->private; > + > + spin_unlock(&clp->cl_lock); > +} > + > +static int opens_show(struct seq_file *s, void *v) > +{ > + struct nfs4_stid *st = v; > + struct nfs4_ol_stateid *os; > + u64 stateid; > + > + if (st->sc_type != NFS4_OPEN_STID) > + return 0; /* XXX: or SEQ_SKIP? */ > + os = openlockstateid(st); > + /* XXX: get info about file, etc., here */ > + > + memcpy(&stateid, &st->sc_stateid, sizeof(stateid)); > + seq_printf(s, "stateid: %llx\n", stateid); > + return 0; > +} More bikeshedding: should we have a "states" file instead of an "opens" file and print a different set of output for each stateid type? > + > +static struct seq_operations opens_seq_ops = { > + .start = opens_start, > + .next = opens_next, > + .stop = opens_stop, > + .show = opens_show > +}; > + > +static int client_opens_open(struct inode *inode, struct file *file) > +{ > + struct nfsdfs_client *nc; > + struct seq_file *s; > + struct nfs4_client *clp; > + int ret; > + > + nc = get_nfsdfs_client(inode); > + if (!nc) > + return -ENXIO; > + clp = container_of(nc, struct nfs4_client, cl_nfsdfs); > + > + ret = seq_open(file, &opens_seq_ops); > + if (ret) > + return ret; > + s = file->private_data; > + s->private = clp; > + return 0; > +} > + > +static int client_opens_release(struct inode *inode, struct file *file) > +{ > + struct seq_file *m = file->private_data; > + struct nfs4_client *clp = m->private; > + > + /* XXX: alternatively, we could get/drop in seq start/stop */ > + drop_client(clp); > + return 0; > +} > + > +static const struct file_operations client_opens_fops = { > + .open = client_opens_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = client_opens_release, > +}; > + > static const struct tree_descr client_files[] = { > [0] = {"info", &client_info_fops, S_IRUSR}, > - [1] = {""}, > + [1] = {"open", &client_opens_fops, S_IRUSR}, > + [2] = {""}, > }; > > static struct nfs4_client *create_client(struct xdr_netobj name,
On Thu, Apr 25, 2019 at 02:04:59PM -0400, Jeff Layton wrote: > More bikeshedding: should we have a "states" file instead of an "opens" > file and print a different set of output for each stateid type? Sure. The format of the file could be something like <stateid> open rw -- <openowner>... <stateid> lock r 0-EOF <lockowner>... <stateid> deleg r I wonder if we could put owners on separate lines and do some heirarchical thing to show owner-stateid relationships? Hm. That's kind of appealing but more work. I was only planning to do opens for the first iteration, and I think extending later in separate files is slightly safer. More trivial, but: it'd lengthen lines and make columns line up less often. But if we include a lot of long variable-length fields then that's kinda hopeless anyway. --b.
On Apr 25, 2019, at 10:14 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > On Thu, Apr 25, 2019 at 02:04:59PM -0400, Jeff Layton wrote: >> More bikeshedding: should we have a "states" file instead of an "opens" >> file and print a different set of output for each stateid type? > > Sure. The format of the file could be something like > > <stateid> open rw -- <openowner>... > <stateid> lock r 0-EOF <lockowner>... > <stateid> deleg r > > I wonder if we could put owners on separate lines and do some > heirarchical thing to show owner-stateid relationships? Hm. That's > kind of appealing but more work. My suggestion here would be to use YAML-formatted output rather than space/tab separated positional fields. That can still be made human readable, but also machine parseable and extensible if formatted properly. Something like the following (use https://yaml-online-parser.appspot.com/ to validate the formatting): - <stateid>: { state: open, mode: rw, owner: <openowner> } - <stateid>: { state: lock, mode: r, start: 0, end: EOF, owner: <openowner> } > I was only planning to do opens for the first iteration, and I think > extending later in separate files is slightly safer. > > More trivial, but: it'd lengthen lines and make columns line up less > often. But if we include a lot of long variable-length fields then > that's kinda hopeless anyway. > > --b. Cheers, Andreas
On Thu, Apr 25, 2019 at 11:14:23PM +0200, Andreas Dilger wrote: > On Apr 25, 2019, at 10:14 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > On Thu, Apr 25, 2019 at 02:04:59PM -0400, Jeff Layton wrote: > >> More bikeshedding: should we have a "states" file instead of an "opens" > >> file and print a different set of output for each stateid type? > > > > Sure. The format of the file could be something like > > > > <stateid> open rw -- <openowner>... > > <stateid> lock r 0-EOF <lockowner>... > > <stateid> deleg r > > > > I wonder if we could put owners on separate lines and do some > > heirarchical thing to show owner-stateid relationships? Hm. That's > > kind of appealing but more work. > > My suggestion here would be to use YAML-formatted output rather than > space/tab separated positional fields. That can still be made human > readable, but also machine parseable and extensible if formatted properly. Well, anything we do will be machine-parseable. But I can believe YAML would make future extension easier. It doesn't look like it would be more complicated to generate. It uses C-style escaping (like \x32) so there'd be no change to how we format binary blobs. The field names make it a tad more verbose but I guess it's not too bad. > Something like the following (use https://yaml-online-parser.appspot.com/ > to validate the formatting): > > - <stateid>: { state: open, mode: rw, owner: <openowner> } > - <stateid>: { state: lock, mode: r, start: 0, end: EOF, owner: <openowner> } I haven't noticed other kernel interfaces using YAML. But I guess I'm not coming up with any real objection. --b.
On Thu, Apr 25, 2019 at 09:18:04PM -0400, J. Bruce Fields wrote: > On Thu, Apr 25, 2019 at 11:14:23PM +0200, Andreas Dilger wrote: > > On Apr 25, 2019, at 10:14 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > > > On Thu, Apr 25, 2019 at 02:04:59PM -0400, Jeff Layton wrote: > > >> More bikeshedding: should we have a "states" file instead of an "opens" > > >> file and print a different set of output for each stateid type? > > > > > > Sure. The format of the file could be something like > > > > > > <stateid> open rw -- <openowner>... > > > <stateid> lock r 0-EOF <lockowner>... > > > <stateid> deleg r > > > > > > I wonder if we could put owners on separate lines and do some > > > heirarchical thing to show owner-stateid relationships? Hm. That's > > > kind of appealing but more work. > > > > My suggestion here would be to use YAML-formatted output rather than > > space/tab separated positional fields. That can still be made human > > readable, but also machine parseable and extensible if formatted properly. > > Well, anything we do will be machine-parseable. But I can believe YAML > would make future extension easier. It doesn't look like it would be > more complicated to generate. It uses C-style escaping (like \x32) so > there'd be no change to how we format binary blobs. > > The field names make it a tad more verbose but I guess it's not too bad. OK, I tried changing "opens" to "states" and using YAML. Example output: - 0x020000006a5fdc5c4ad09d9e01000000: { type: open, access: rw, deny: --, superblock: "fd:10:13649", owner: "open id:\x00\x00\x00&\x00\x00\x00\x00\x00\x0046��QH " } - 0x010000006a5fdc5c4ad09d9e03000000: { type: open, access: r-, deny: --, superblock: "fd:10:13650", owner: "open id:\x00\x00\x00&\x00\x00\x00\x00\x00\x0046��QH" } - 0x010000006a5fdc5c4ad09d9e04000000: { type: deleg, access: r, superblock: "fd:10:13650" } - 0x010000006a5fdc5c4ad09d9e06000000: { type: lock, superblock: "fd:10:13649", owner: "lock id:\x00\x00\x00&\x00\x00\x00\x00\x00\x00\x00\x00" } The parser Andreas suggested (https://yaml-online-parser.appspot.com/) accepts these. It also thinks strings are always in a unicode encoding of some kind, which they aren't. The owners are arbitrary series of bytes but I'd like at least any ascii parts to be human readable, and I'm a little stuck on how to do that. --b.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 928705fc8ff5..829d1e5440d3 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -684,7 +684,8 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla idr_preload(GFP_KERNEL); spin_lock(&cl->cl_lock); - new_id = idr_alloc_cyclic(&cl->cl_stateids, stid, 0, 0, GFP_NOWAIT); + /* Reserving 0 for start of file in nfsdfs "opens" file: */ + new_id = idr_alloc_cyclic(&cl->cl_stateids, stid, 1, 0, GFP_NOWAIT); spin_unlock(&cl->cl_lock); idr_preload_end(); if (new_id < 0) @@ -2223,9 +2224,104 @@ static const struct file_operations client_info_fops = { .release = single_release, }; +static void *opens_start(struct seq_file *s, loff_t *pos) + __acquires(&clp->cl_lock) +{ + struct nfs4_client *clp = s->private; + unsigned long id = *pos; + void *ret; + + spin_lock(&clp->cl_lock); + ret = idr_get_next_ul(&clp->cl_stateids, &id); + *pos = id; + return ret; +} + +static void *opens_next(struct seq_file *s, void *v, loff_t *pos) +{ + struct nfs4_client *clp = s->private; + unsigned long id = *pos; + void *ret; + + id = *pos; + id++; + ret = idr_get_next_ul(&clp->cl_stateids, &id); + *pos = id; + return ret; +} + +static void opens_stop(struct seq_file *s, void *v) + __releases(&clp->cl_lock) +{ + struct nfs4_client *clp = s->private; + + spin_unlock(&clp->cl_lock); +} + +static int opens_show(struct seq_file *s, void *v) +{ + struct nfs4_stid *st = v; + struct nfs4_ol_stateid *os; + u64 stateid; + + if (st->sc_type != NFS4_OPEN_STID) + return 0; /* XXX: or SEQ_SKIP? */ + os = openlockstateid(st); + /* XXX: get info about file, etc., here */ + + memcpy(&stateid, &st->sc_stateid, sizeof(stateid)); + seq_printf(s, "stateid: %llx\n", stateid); + return 0; +} + +static struct seq_operations opens_seq_ops = { + .start = opens_start, + .next = opens_next, + .stop = opens_stop, + .show = opens_show +}; + +static int client_opens_open(struct inode *inode, struct file *file) +{ + struct nfsdfs_client *nc; + struct seq_file *s; + struct nfs4_client *clp; + int ret; + + nc = get_nfsdfs_client(inode); + if (!nc) + return -ENXIO; + clp = container_of(nc, struct nfs4_client, cl_nfsdfs); + + ret = seq_open(file, &opens_seq_ops); + if (ret) + return ret; + s = file->private_data; + s->private = clp; + return 0; +} + +static int client_opens_release(struct inode *inode, struct file *file) +{ + struct seq_file *m = file->private_data; + struct nfs4_client *clp = m->private; + + /* XXX: alternatively, we could get/drop in seq start/stop */ + drop_client(clp); + return 0; +} + +static const struct file_operations client_opens_fops = { + .open = client_opens_open, + .read = seq_read, + .llseek = seq_lseek, + .release = client_opens_release, +}; + static const struct tree_descr client_files[] = { [0] = {"info", &client_info_fops, S_IRUSR}, - [1] = {""}, + [1] = {"open", &client_opens_fops, S_IRUSR}, + [2] = {""}, }; static struct nfs4_client *create_client(struct xdr_netobj name,