| Message ID | 87sg4rerta.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mountd/exportd: only log confirmed clients, and poll for updates | expand |
On Fri, Mar 19, 2021 at 02:38:25PM +1100, NeilBrown wrote: > > It is possible (and common with the Linux NFS client) for the nfs server > to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting > a connection. This results in some clients appearing in > /proc/fs/nfsd/clients > which never get confirmed. mountd currently logs these, but they aren't > really helpful. > > If the kernel supports the reporting of the confirmation status of > clients, we can suppress the message until a client is confirmed. > > With this patch we: > - record if the client is confirmed, assuming it is if the status is > not reported > - don't log unconfirmed clients > - check all unconfirmed clients each time any event is processed, > - if there are unconfirmed clients, we request a wakeup after a > exponentially decreasing time, and check again increasing not decreasing, I think. Is there any better way to let userland know when the contents of a virtual file have changed? Looks at inofity man page.... There's an "IN_MODIFY" event. I think we could add an fsnotify_inode(inode, FS_MODIFY); at the end of move_to_confirmed(). (I'm not sure what's the best way to get the inode of the info file there.) Would that help? --b. > This requires updating the two event loops to allow a timeout to be > specified. > > When there is no other activity, but there are unconfirmed clients, > timeout for repeat checks are after 1, 2, 4 ... 512 seconds. > If any wait is interrupted, the next wait time is still chosen, so we > might advance quickly through the list. But if there is lots of > activity, we don't really need the timeout. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > support/export/cache.c | 7 ++- > support/export/export.h | 2 +- > support/export/v4clients.c | 92 ++++++++++++++++++++++++++++++-------- > utils/mountd/svc_run.c | 7 ++- > 4 files changed, 85 insertions(+), 23 deletions(-) > > diff --git a/support/export/cache.c b/support/export/cache.c > index 3e4f53c0a32e..687b3aef27a1 100644 > --- a/support/export/cache.c > +++ b/support/export/cache.c > @@ -1524,6 +1524,7 @@ void cache_process_loop(void) > { > fd_set readfds; > int selret; > + struct timeval tv = { 60*60, 0 }; > > FD_ZERO(&readfds); > > @@ -1533,8 +1534,10 @@ void cache_process_loop(void) > v4clients_set_fds(&readfds); > > selret = select(FD_SETSIZE, &readfds, > - (void *) 0, (void *) 0, (struct timeval *) 0); > + (void *) 0, (void *) 0, &tv); > > + tv.tv_sec = 60*60; > + tv.tv_usec = 0; > > switch (selret) { > case -1: > @@ -1546,7 +1549,7 @@ void cache_process_loop(void) > > default: > cache_process_req(&readfds); > - v4clients_process(&readfds); > + v4clients_process(&readfds, &tv); > } > } > } > diff --git a/support/export/export.h b/support/export/export.h > index 8d5a0d3004ef..71d3ed39bd28 100644 > --- a/support/export/export.h > +++ b/support/export/export.h > @@ -24,7 +24,7 @@ void cache_process_loop(void); > > void v4clients_init(void); > void v4clients_set_fds(fd_set *fdset); > -int v4clients_process(fd_set *fdset); > +int v4clients_process(fd_set *fdset, struct timeval *tv); > > struct nfs_fh_len * > cache_get_filehandle(nfs_export *exp, int len, char *p); > diff --git a/support/export/v4clients.c b/support/export/v4clients.c > index 056ddc9b065d..04cec3136876 100644 > --- a/support/export/v4clients.c > +++ b/support/export/v4clients.c > @@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset) > } > > static void *tree_root; > +static int have_unconfirmed; > +static time_t unconfirmed_wait = 1; > > struct ent { > unsigned long num; > char *clientid; > char *addr; > int vers; > + int unconfirmed; > }; > > static int ent_cmp(const void *av, const void *bv) > @@ -89,15 +92,13 @@ static char *dup_line(char *line) > return ret; > } > > -static void add_id(int id) > +static void read_info(struct ent *key) > { > char buf[2048]; > - struct ent **ent; > - struct ent *key; > char *path; > FILE *f; > > - if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0) > + if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0) > return; > > f = fopen(path, "r"); > @@ -105,32 +106,56 @@ static void add_id(int id) > free(path); > return; > } > - key = calloc(1, sizeof(*key)); > - if (!key) { > - fclose(f); > - free(path); > - return; > - } > - key->num = id; > while (fgets(buf, sizeof(buf), f)) { > - if (strncmp(buf, "clientid: ", 10) == 0) > + if (strncmp(buf, "clientid: ", 10) == 0) { > + free(key->clientid); > key->clientid = dup_line(buf+10); > - if (strncmp(buf, "address: ", 9) == 0) > + } > + if (strncmp(buf, "address: ", 9) == 0) { > + free(key->addr); > key->addr = dup_line(buf+9); > + } > if (strncmp(buf, "minor version: ", 15) == 0) > key->vers = atoi(buf+15); > + if (strncmp(buf, "status: ", 8) == 0 && > + strstr(buf, " unconfirmed") != NULL) { > + key->unconfirmed = 1; > + have_unconfirmed = 1; > + } > + if (strncmp(buf, "status: ", 8) == 0 && > + strstr(buf, " confirmed") != NULL) > + key->unconfirmed = 0; > } > fclose(f); > free(path); > > - xlog(L_NOTICE, "v4.%d client attached: %s from %s", > - key->vers, key->clientid, key->addr); > + if (!key->unconfirmed) > + xlog(L_NOTICE, "v4.%d client attached: %s from %s", > + key->vers, key->clientid ?: "-none-", > + key->addr ?: "-none-"); > +} > + > +static void add_id(int id) > +{ > + struct ent **ent; > + struct ent *key; > + > + key = calloc(1, sizeof(*key)); > + if (!key) { > + return; > + } > + key->num = id; > > ent = tsearch(key, &tree_root, ent_cmp); > > if (!ent || *ent != key) > /* Already existed, or insertion failed */ > free_ent(key); > + else { > + read_info(key); > + if (key->unconfirmed) > + unconfirmed_wait = 1; > + } > } > > static void del_id(int id) > @@ -143,18 +168,43 @@ static void del_id(int id) > return; > ent = *e; > tdelete(ent, &tree_root, ent_cmp); > - xlog(L_NOTICE, "v4.%d client detached: %s from %s", > - ent->vers, ent->clientid, ent->addr); > + if (!ent->unconfirmed) > + xlog(L_NOTICE, "v4.%d client detached: %s from %s", > + ent->vers, ent->clientid, ent->addr); > free_ent(ent); > } > > -int v4clients_process(fd_set *fdset) > +static void check_one(const void *ventp, VISIT which, int depth) > +{ > + struct ent * const *ep = ventp; > + struct ent *ent; > + > + if (depth >= 0 && /* Silence "unused paramter" warning */ > + which != preorder && which != leaf) > + return; > + > + ent = *ep; > + if (ent->unconfirmed) > + read_info(ent); > +} > + > +static void check_unconfirmed(void) > +{ > + if (!have_unconfirmed) > + return; > + have_unconfirmed = 0; > + twalk(tree_root, check_one); > +} > + > +int v4clients_process(fd_set *fdset, struct timeval *tv) > { > char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event)))); > const struct inotify_event *ev; > ssize_t len; > char *ptr; > > + check_unconfirmed(); > + > if (clients_fd < 0 || > !FD_ISSET(clients_fd, fdset)) > return 0; > @@ -174,6 +224,12 @@ int v4clients_process(fd_set *fdset) > del_id(id); > } > } > + if (have_unconfirmed && tv->tv_sec > unconfirmed_wait) { > + tv->tv_sec = unconfirmed_wait; > + tv->tv_usec = 0; > + if (unconfirmed_wait < 300) > + unconfirmed_wait <<= 1; > + } > return 1; > } > > diff --git a/utils/mountd/svc_run.c b/utils/mountd/svc_run.c > index 167b9757bde2..042bed8957b9 100644 > --- a/utils/mountd/svc_run.c > +++ b/utils/mountd/svc_run.c > @@ -95,6 +95,7 @@ my_svc_run(void) > { > fd_set readfds; > int selret; > + struct timeval tv = { 60*60, 0 }; > > for (;;) { > > @@ -103,8 +104,10 @@ my_svc_run(void) > v4clients_set_fds(&readfds); > > selret = select(FD_SETSIZE, &readfds, > - (void *) 0, (void *) 0, (struct timeval *) 0); > + (void *) 0, (void *) 0, &tv); > > + tv.tv_sec = 60*60; > + tv.tv_usec = 0; > > switch (selret) { > case -1: > @@ -116,7 +119,7 @@ my_svc_run(void) > > default: > selret -= cache_process_req(&readfds); > - selret -= v4clients_process(&readfds); > + selret -= v4clients_process(&readfds, &tv); > if (selret) > svc_getreqset(&readfds); > } > -- > 2.30.1 >
On Fri, Mar 19 2021, J. Bruce Fields wrote: > On Fri, Mar 19, 2021 at 02:38:25PM +1100, NeilBrown wrote: >> >> It is possible (and common with the Linux NFS client) for the nfs server >> to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting >> a connection. This results in some clients appearing in >> /proc/fs/nfsd/clients >> which never get confirmed. mountd currently logs these, but they aren't >> really helpful. >> >> If the kernel supports the reporting of the confirmation status of >> clients, we can suppress the message until a client is confirmed. >> >> With this patch we: >> - record if the client is confirmed, assuming it is if the status is >> not reported >> - don't log unconfirmed clients >> - check all unconfirmed clients each time any event is processed, >> - if there are unconfirmed clients, we request a wakeup after a >> exponentially decreasing time, and check again > > increasing not decreasing, I think. or frequency, not time?? Thanks. > > Is there any better way to let userland know when the contents of a > virtual file have changed? i had thought of using sysfs_notify_dirent(), though the file isn't in sysfs (or kernfs), so extra work might be needed. And I didn't want to hold the file descriptor open, as then I would need to worry about where the available fds could be exhausted. > > Looks at inofity man page.... There's an "IN_MODIFY" event. I think we > could add an fsnotify_inode(inode, FS_MODIFY); at the end of > move_to_confirmed(). (I'm not sure what's the best way to get the inode > of the info file there.) > > Would that help? Yes, that is a much better idea. We could pass an array of dentry pointers to nfsd_client_mkdir and thence to nfsdfs_create_files. Then create_client could grab the required dentry and stash it next to cl_nfsd_dentry. Then use fsnotify_dirent() which is better than _inode as the file name gets included in the event. I might give that a try. Thanks, NeilBrown
diff --git a/support/export/cache.c b/support/export/cache.c index 3e4f53c0a32e..687b3aef27a1 100644 --- a/support/export/cache.c +++ b/support/export/cache.c @@ -1524,6 +1524,7 @@ void cache_process_loop(void) { fd_set readfds; int selret; + struct timeval tv = { 60*60, 0 }; FD_ZERO(&readfds); @@ -1533,8 +1534,10 @@ void cache_process_loop(void) v4clients_set_fds(&readfds); selret = select(FD_SETSIZE, &readfds, - (void *) 0, (void *) 0, (struct timeval *) 0); + (void *) 0, (void *) 0, &tv); + tv.tv_sec = 60*60; + tv.tv_usec = 0; switch (selret) { case -1: @@ -1546,7 +1549,7 @@ void cache_process_loop(void) default: cache_process_req(&readfds); - v4clients_process(&readfds); + v4clients_process(&readfds, &tv); } } } diff --git a/support/export/export.h b/support/export/export.h index 8d5a0d3004ef..71d3ed39bd28 100644 --- a/support/export/export.h +++ b/support/export/export.h @@ -24,7 +24,7 @@ void cache_process_loop(void); void v4clients_init(void); void v4clients_set_fds(fd_set *fdset); -int v4clients_process(fd_set *fdset); +int v4clients_process(fd_set *fdset, struct timeval *tv); struct nfs_fh_len * cache_get_filehandle(nfs_export *exp, int len, char *p); diff --git a/support/export/v4clients.c b/support/export/v4clients.c index 056ddc9b065d..04cec3136876 100644 --- a/support/export/v4clients.c +++ b/support/export/v4clients.c @@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset) } static void *tree_root; +static int have_unconfirmed; +static time_t unconfirmed_wait = 1; struct ent { unsigned long num; char *clientid; char *addr; int vers; + int unconfirmed; }; static int ent_cmp(const void *av, const void *bv) @@ -89,15 +92,13 @@ static char *dup_line(char *line) return ret; } -static void add_id(int id) +static void read_info(struct ent *key) { char buf[2048]; - struct ent **ent; - struct ent *key; char *path; FILE *f; - if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0) + if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0) return; f = fopen(path, "r"); @@ -105,32 +106,56 @@ static void add_id(int id) free(path); return; } - key = calloc(1, sizeof(*key)); - if (!key) { - fclose(f); - free(path); - return; - } - key->num = id; while (fgets(buf, sizeof(buf), f)) { - if (strncmp(buf, "clientid: ", 10) == 0) + if (strncmp(buf, "clientid: ", 10) == 0) { + free(key->clientid); key->clientid = dup_line(buf+10); - if (strncmp(buf, "address: ", 9) == 0) + } + if (strncmp(buf, "address: ", 9) == 0) { + free(key->addr); key->addr = dup_line(buf+9); + } if (strncmp(buf, "minor version: ", 15) == 0) key->vers = atoi(buf+15); + if (strncmp(buf, "status: ", 8) == 0 && + strstr(buf, " unconfirmed") != NULL) { + key->unconfirmed = 1; + have_unconfirmed = 1; + } + if (strncmp(buf, "status: ", 8) == 0 && + strstr(buf, " confirmed") != NULL) + key->unconfirmed = 0; } fclose(f); free(path); - xlog(L_NOTICE, "v4.%d client attached: %s from %s", - key->vers, key->clientid, key->addr); + if (!key->unconfirmed) + xlog(L_NOTICE, "v4.%d client attached: %s from %s", + key->vers, key->clientid ?: "-none-", + key->addr ?: "-none-"); +} + +static void add_id(int id) +{ + struct ent **ent; + struct ent *key; + + key = calloc(1, sizeof(*key)); + if (!key) { + return; + } + key->num = id; ent = tsearch(key, &tree_root, ent_cmp); if (!ent || *ent != key) /* Already existed, or insertion failed */ free_ent(key); + else { + read_info(key); + if (key->unconfirmed) + unconfirmed_wait = 1; + } } static void del_id(int id) @@ -143,18 +168,43 @@ static void del_id(int id) return; ent = *e; tdelete(ent, &tree_root, ent_cmp); - xlog(L_NOTICE, "v4.%d client detached: %s from %s", - ent->vers, ent->clientid, ent->addr); + if (!ent->unconfirmed) + xlog(L_NOTICE, "v4.%d client detached: %s from %s", + ent->vers, ent->clientid, ent->addr); free_ent(ent); } -int v4clients_process(fd_set *fdset) +static void check_one(const void *ventp, VISIT which, int depth) +{ + struct ent * const *ep = ventp; + struct ent *ent; + + if (depth >= 0 && /* Silence "unused paramter" warning */ + which != preorder && which != leaf) + return; + + ent = *ep; + if (ent->unconfirmed) + read_info(ent); +} + +static void check_unconfirmed(void) +{ + if (!have_unconfirmed) + return; + have_unconfirmed = 0; + twalk(tree_root, check_one); +} + +int v4clients_process(fd_set *fdset, struct timeval *tv) { char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event)))); const struct inotify_event *ev; ssize_t len; char *ptr; + check_unconfirmed(); + if (clients_fd < 0 || !FD_ISSET(clients_fd, fdset)) return 0; @@ -174,6 +224,12 @@ int v4clients_process(fd_set *fdset) del_id(id); } } + if (have_unconfirmed && tv->tv_sec > unconfirmed_wait) { + tv->tv_sec = unconfirmed_wait; + tv->tv_usec = 0; + if (unconfirmed_wait < 300) + unconfirmed_wait <<= 1; + } return 1; } diff --git a/utils/mountd/svc_run.c b/utils/mountd/svc_run.c index 167b9757bde2..042bed8957b9 100644 --- a/utils/mountd/svc_run.c +++ b/utils/mountd/svc_run.c @@ -95,6 +95,7 @@ my_svc_run(void) { fd_set readfds; int selret; + struct timeval tv = { 60*60, 0 }; for (;;) { @@ -103,8 +104,10 @@ my_svc_run(void) v4clients_set_fds(&readfds); selret = select(FD_SETSIZE, &readfds, - (void *) 0, (void *) 0, (struct timeval *) 0); + (void *) 0, (void *) 0, &tv); + tv.tv_sec = 60*60; + tv.tv_usec = 0; switch (selret) { case -1: @@ -116,7 +119,7 @@ my_svc_run(void) default: selret -= cache_process_req(&readfds); - selret -= v4clients_process(&readfds); + selret -= v4clients_process(&readfds, &tv); if (selret) svc_getreqset(&readfds); }
It is possible (and common with the Linux NFS client) for the nfs server to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting a connection. This results in some clients appearing in /proc/fs/nfsd/clients which never get confirmed. mountd currently logs these, but they aren't really helpful. If the kernel supports the reporting of the confirmation status of clients, we can suppress the message until a client is confirmed. With this patch we: - record if the client is confirmed, assuming it is if the status is not reported - don't log unconfirmed clients - check all unconfirmed clients each time any event is processed, - if there are unconfirmed clients, we request a wakeup after a exponentially decreasing time, and check again This requires updating the two event loops to allow a timeout to be specified. When there is no other activity, but there are unconfirmed clients, timeout for repeat checks are after 1, 2, 4 ... 512 seconds. If any wait is interrupted, the next wait time is still chosen, so we might advance quickly through the list. But if there is lots of activity, we don't really need the timeout. Signed-off-by: NeilBrown <neilb@suse.de> --- support/export/cache.c | 7 ++- support/export/export.h | 2 +- support/export/v4clients.c | 92 ++++++++++++++++++++++++++++++-------- utils/mountd/svc_run.c | 7 ++- 4 files changed, 85 insertions(+), 23 deletions(-)