| Message ID | 20241119004928.3245873-4-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: allocate/free session-based DRC slots on demand | expand |
On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > Each client now reports the number of slots allocated in each session. Can this file also report the target slot count? Ie, is the server matching the client's requested slot count, or is it over or under by some number? Would it be useful for a server tester or administrator to poke a target slot count value into this file and watch the machinery adjust? > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 3889ba1c653f..31ff9f92a895 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) > static int client_info_show(struct seq_file *m, void *v) > { > struct inode *inode = file_inode(m->file); > + struct nfsd4_session *ses; > struct nfs4_client *clp; > u64 clid; > > @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) > seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); > seq_printf(m, "admin-revoked states: %d\n", > atomic_read(&clp->cl_admin_revoked)); > + seq_printf(m, "session slots:"); > + spin_lock(&clp->cl_lock); > + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) > + seq_printf(m, " %u", ses->se_fchannel.maxreqs); > + spin_unlock(&clp->cl_lock); > + seq_puts(m, "\n"); > + > drop_client(clp); > > return 0; > -- > 2.47.0 >
On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > Each client now reports the number of slots allocated in each session. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 3889ba1c653f..31ff9f92a895 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) > static int client_info_show(struct seq_file *m, void *v) > { > struct inode *inode = file_inode(m->file); > + struct nfsd4_session *ses; > struct nfs4_client *clp; > u64 clid; > > @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) > seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); > seq_printf(m, "admin-revoked states: %d\n", > atomic_read(&clp->cl_admin_revoked)); > + seq_printf(m, "session slots:"); > + spin_lock(&clp->cl_lock); > + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) > + seq_printf(m, " %u", ses->se_fchannel.maxreqs); > + spin_unlock(&clp->cl_lock); > + seq_puts(m, "\n"); > + Also, I wonder if information about the backchannel session can be surfaced in this way? > drop_client(clp); > > return 0; > -- > 2.47.0 >
On Wed, 20 Nov 2024, Chuck Lever wrote: > On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > > Each client now reports the number of slots allocated in each session. > > Can this file also report the target slot count? Ie, is the server > matching the client's requested slot count, or is it over or under > by some number? I could. Would you like to suggest a syntax? Usually the numbers would be the same except for short transition periods, so I'm not convinced of the value. Currently if the target is reduced while the client is idle there can be a longer delay before the slots are actually freed, but I think 2 lease-renewal SEQUENCE ops would do it. If/when we add use of the CB_RECALL_SLOT callback the delay should disappear. > > Would it be useful for a server tester or administrator to poke a > target slot count value into this file and watch the machinery > adjust? Maybe. By echo 3 > drop_caches does a pretty good job. I don't see that we need more. Thanks, NeilBrown
On Wed, 20 Nov 2024, Chuck Lever wrote: > On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > > Each client now reports the number of slots allocated in each session. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfs4state.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 3889ba1c653f..31ff9f92a895 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) > > static int client_info_show(struct seq_file *m, void *v) > > { > > struct inode *inode = file_inode(m->file); > > + struct nfsd4_session *ses; > > struct nfs4_client *clp; > > u64 clid; > > > > @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) > > seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); > > seq_printf(m, "admin-revoked states: %d\n", > > atomic_read(&clp->cl_admin_revoked)); > > + seq_printf(m, "session slots:"); > > + spin_lock(&clp->cl_lock); > > + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) > > + seq_printf(m, " %u", ses->se_fchannel.maxreqs); > > + spin_unlock(&clp->cl_lock); > > + seq_puts(m, "\n"); > > + > > Also, I wonder if information about the backchannel session can be > surfaced in this way? > Probably make sense. Maybe we should invent a syntax for reporting arbitrary info about each session. session %d slots: %d session %d cb-slots: %d ... ??? NeilBrown
On Wed, Nov 20, 2024 at 09:22:59AM +1100, NeilBrown wrote: > On Wed, 20 Nov 2024, Chuck Lever wrote: > > On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > > > Each client now reports the number of slots allocated in each session. > > > > Can this file also report the target slot count? Ie, is the server > > matching the client's requested slot count, or is it over or under > > by some number? > > I could. Would you like to suggest a syntax? > Usually the numbers would be the same except for short transition > periods, so I'm not convinced of the value. That's precisely the kind of situation I would like to be able catch -- the two are unequal longer than expected. > Currently if the target is reduced while the client is idle there can be > a longer delay before the slots are actually freed, but I think 2 > lease-renewal SEQUENCE ops would do it. If/when we add use of the > CB_RECALL_SLOT callback the delay should disappear. > > > Would it be useful for a server tester or administrator to poke a > > target slot count value into this file and watch the machinery > > adjust? > > Maybe. By echo 3 > drop_caches does a pretty good job. I don't see > that we need more. Fair enough.
On Wed, Nov 20, 2024 at 09:24:52AM +1100, NeilBrown wrote: > On Wed, 20 Nov 2024, Chuck Lever wrote: > > On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > > > Each client now reports the number of slots allocated in each session. > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > fs/nfsd/nfs4state.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > index 3889ba1c653f..31ff9f92a895 100644 > > > --- a/fs/nfsd/nfs4state.c > > > +++ b/fs/nfsd/nfs4state.c > > > @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) > > > static int client_info_show(struct seq_file *m, void *v) > > > { > > > struct inode *inode = file_inode(m->file); > > > + struct nfsd4_session *ses; > > > struct nfs4_client *clp; > > > u64 clid; > > > > > > @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) > > > seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); > > > seq_printf(m, "admin-revoked states: %d\n", > > > atomic_read(&clp->cl_admin_revoked)); > > > + seq_printf(m, "session slots:"); > > > + spin_lock(&clp->cl_lock); > > > + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) > > > + seq_printf(m, " %u", ses->se_fchannel.maxreqs); > > > + spin_unlock(&clp->cl_lock); > > > + seq_puts(m, "\n"); > > > + > > > > Also, I wonder if information about the backchannel session can be > > surfaced in this way? > > > > Probably make sense. Maybe we should invent a syntax for reporting > arbitrary info about each session. > > session %d slots: %d > session %d cb-slots: %d > ... > > ??? If each client has a directory, then it should have a subdirectory called "sessions". Each subdirectory of "sessions" should be one session, named by its hex session ID (as it is presented by Wireshark). Each session directory could have a file for the forward channel, one for the backchannel, and maybe one for generic information like when the session was created and how many connections it has. We don't need all of that in this patch set, but whatever is introduced here should be extensible to allow us to add more over time.
On Wed, 20 Nov 2024, Chuck Lever wrote: > On Wed, Nov 20, 2024 at 09:24:52AM +1100, NeilBrown wrote: > > On Wed, 20 Nov 2024, Chuck Lever wrote: > > > On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: > > > > Each client now reports the number of slots allocated in each session. > > > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > --- > > > > fs/nfsd/nfs4state.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > > index 3889ba1c653f..31ff9f92a895 100644 > > > > --- a/fs/nfsd/nfs4state.c > > > > +++ b/fs/nfsd/nfs4state.c > > > > @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) > > > > static int client_info_show(struct seq_file *m, void *v) > > > > { > > > > struct inode *inode = file_inode(m->file); > > > > + struct nfsd4_session *ses; > > > > struct nfs4_client *clp; > > > > u64 clid; > > > > > > > > @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) > > > > seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); > > > > seq_printf(m, "admin-revoked states: %d\n", > > > > atomic_read(&clp->cl_admin_revoked)); > > > > + seq_printf(m, "session slots:"); > > > > + spin_lock(&clp->cl_lock); > > > > + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) > > > > + seq_printf(m, " %u", ses->se_fchannel.maxreqs); > > > > + spin_unlock(&clp->cl_lock); > > > > + seq_puts(m, "\n"); > > > > + > > > > > > Also, I wonder if information about the backchannel session can be > > > surfaced in this way? > > > > > > > Probably make sense. Maybe we should invent a syntax for reporting > > arbitrary info about each session. > > > > session %d slots: %d > > session %d cb-slots: %d > > ... > > > > ??? > > If each client has a directory, then it should have a subdirectory > called "sessions". Each subdirectory of "sessions" should be one > session, named by its hex session ID (as it is presented by > Wireshark). Each session directory could have a file for the forward > channel, one for the backchannel, and maybe one for generic > information like when the session was created and how many > connections it has. > > We don't need all of that in this patch set, but whatever is > introduced here should be extensible to allow us to add more over > time. I cannot say I'm excited about the proliferation of tiny files. Your suggestion isn't quite as bad as sysfs which claims to want one file per value, but I think the sysfs approach provided more pain than gain and you seem to be heading that way. As evidence I present the rise of netlink. Netlink's main advantage is that it allows you to access a collection of data in a single syscall (or maybe pair of syscalls). If we had a standard format for doing that with open/read/close, the filesystem would be a much nicer interface. But the sysfs rules prevent that, so people who care avoid it. We don't need to impose those same rules on nfsd-fs. Having separate dirs for the clients makes some sense as the clients are quite independent. Sessions aren't - they are just part of the client. The *only* way session information is different from other client information is that there is more structure - an array of sessions each with detail. I don't think that justifies a new directory. I does justify a carefully designed (or chosen) format for representing structured data. Thanks, NeilBrown
> On Nov 21, 2024, at 4:03 PM, NeilBrown <neilb@suse.de> wrote: > > On Wed, 20 Nov 2024, Chuck Lever wrote: >> On Wed, Nov 20, 2024 at 09:24:52AM +1100, NeilBrown wrote: >>> On Wed, 20 Nov 2024, Chuck Lever wrote: >>>> On Tue, Nov 19, 2024 at 11:41:30AM +1100, NeilBrown wrote: >>>>> Each client now reports the number of slots allocated in each session. >>>>> >>>>> Signed-off-by: NeilBrown <neilb@suse.de> >>>>> --- >>>>> fs/nfsd/nfs4state.c | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>>>> index 3889ba1c653f..31ff9f92a895 100644 >>>>> --- a/fs/nfsd/nfs4state.c >>>>> +++ b/fs/nfsd/nfs4state.c >>>>> @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) >>>>> static int client_info_show(struct seq_file *m, void *v) >>>>> { >>>>> struct inode *inode = file_inode(m->file); >>>>> + struct nfsd4_session *ses; >>>>> struct nfs4_client *clp; >>>>> u64 clid; >>>>> >>>>> @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) >>>>> seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); >>>>> seq_printf(m, "admin-revoked states: %d\n", >>>>> atomic_read(&clp->cl_admin_revoked)); >>>>> + seq_printf(m, "session slots:"); >>>>> + spin_lock(&clp->cl_lock); >>>>> + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) >>>>> + seq_printf(m, " %u", ses->se_fchannel.maxreqs); >>>>> + spin_unlock(&clp->cl_lock); >>>>> + seq_puts(m, "\n"); >>>>> + >>>> >>>> Also, I wonder if information about the backchannel session can be >>>> surfaced in this way? >>>> >>> >>> Probably make sense. Maybe we should invent a syntax for reporting >>> arbitrary info about each session. >>> >>> session %d slots: %d >>> session %d cb-slots: %d >>> ... >>> >>> ??? >> >> If each client has a directory, then it should have a subdirectory >> called "sessions". Each subdirectory of "sessions" should be one >> session, named by its hex session ID (as it is presented by >> Wireshark). Each session directory could have a file for the forward >> channel, one for the backchannel, and maybe one for generic >> information like when the session was created and how many >> connections it has. >> >> We don't need all of that in this patch set, but whatever is >> introduced here should be extensible to allow us to add more over >> time. > > I cannot say I'm excited about the proliferation of tiny files. Your > suggestion isn't quite as bad as sysfs which claims to want one file per > value, but I think the sysfs approach provided more pain than gain and > you seem to be heading that way. As evidence I present the rise of > netlink. Netlink's main advantage is that it allows you to access a > collection of data in a single syscall (or maybe pair of syscalls). If > we had a standard format for doing that with open/read/close, the > filesystem would be a much nicer interface. But the sysfs rules prevent > that, so people who care avoid it. I don't see this set of information as being in a performance path. Needing multiple open/read/close iterations doesn't seem like an impediment to me. The only possible issue is that user space might want a snapshot of certain related values, and having to get the values from multiple files means there's no guarantee that the values are consistent with each other. > We don't need to impose those same rules on nfsd-fs. > > Having separate dirs for the clients makes some sense as the clients are > quite independent. Sessions aren't - they are just part of the client. > The *only* way session information is different from other client > information is that there is more structure - an array of sessions each > with detail. I don't think that justifies a new directory. Hrm. IMHO a directory is exactly suited to this kind of information hierarchy. I can't say that I understand your view; perhaps you feel this way because the client implementations we are familiar with use only a single session. For that, of course, a directory is overkill. > It does > justify a carefully designed (or chosen) format for representing > structured data. That usually means JSON or XML, which also have their haters. However, I don't feel strongly about this. You asked me for some thoughts, and here they are, at random. My bottom line is reasonable extensibility -- the ability to provide more information in these files in the future without perturbing current consumers. IME that's been nearly impossible with designs that have one file full of fields that need to be parsed. Should we expose session information via the new NFSD netlink protocol instead? Or a sessions/ directory with one formatted file per session? I'm open to discussion. -- Chuck Lever
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 3889ba1c653f..31ff9f92a895 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -2642,6 +2642,7 @@ static const char *cb_state2str(int state) static int client_info_show(struct seq_file *m, void *v) { struct inode *inode = file_inode(m->file); + struct nfsd4_session *ses; struct nfs4_client *clp; u64 clid; @@ -2678,6 +2679,13 @@ static int client_info_show(struct seq_file *m, void *v) seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr); seq_printf(m, "admin-revoked states: %d\n", atomic_read(&clp->cl_admin_revoked)); + seq_printf(m, "session slots:"); + spin_lock(&clp->cl_lock); + list_for_each_entry(ses, &clp->cl_sessions, se_perclnt) + seq_printf(m, " %u", ses->se_fchannel.maxreqs); + spin_unlock(&clp->cl_lock); + seq_puts(m, "\n"); + drop_client(clp); return 0;
Each client now reports the number of slots allocated in each session. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4state.c | 8 ++++++++ 1 file changed, 8 insertions(+)