Re: [PATCH] cifs: introduce dns_interval mount option

[Tom Talpey <tom@xxxxxxxxxx>]

This change looks good to me. I'm not usually a fan of mount
options, but it's entirely logical here, and the option can
easily be no-oped or tweaked in the future in compatible ways
when the upcall is fully implemented.

Reviewed-by: Tom Talpey <tom@xxxxxxxxxx>

On 6/10/2022 1:16 PM, Enzo Matsumiya wrote:
> On 06/10, Shyam Prasad N wrote:
> > On Fri, Jun 10, 2022 at 4:14 AM Enzo Matsumiya <ematsumiya@xxxxxxx> 
> > wrote:
> > > This patch introduces a `dns_interval' mount option, used to configure
> > > the interval that the DNS resolve worker should be run.
> > >
> > > Enforces the minimum value SMB_DNS_RESOLVE_INTERVAL_MIN (currently 
> > > 120s),
> > > or uses the default SMB_DNS_RESOLVE_INTERVAL_DEFAULT (currently 600s).
> > >
> > > Since this is a mount option, each derived connection from it, e.g. DFS
> > > root targets, will share the same DNS interval from the primary server
> > > since the TCP session options are passed down to them.
> > >
> > > Signed-off-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
> > > ---
> > >  fs/cifs/cifsfs.c     |  3 +++
> > >  fs/cifs/cifsglob.h   |  1 +
> > >  fs/cifs/connect.c    | 20 ++++++++++++++------
> > >  fs/cifs/fs_context.c | 11 +++++++++++
> > >  fs/cifs/fs_context.h |  2 ++
> > >  fs/cifs/sess.c       |  1 +
> > >  6 files changed, 32 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index 325423180fd2..ad980b235699 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -665,6 +665,9 @@ cifs_show_options(struct seq_file *s, struct 
> > > dentry *root)
> > >         if (tcon->ses->server->max_credits != 
> > > SMB2_MAX_CREDITS_AVAILABLE)
> > >                 seq_printf(s, ",max_credits=%u", 
> > > tcon->ses->server->max_credits);
> > >
> > > +       if (tcon->ses->server->dns_interval != 
> > > SMB_DNS_RESOLVE_INTERVAL_DEFAULT)
> > > +               seq_printf(s, ",dns_interval=%u", 
> > > tcon->ses->server->dns_interval);
> > > +
> > >         if (tcon->snapshot_time)
> > >                 seq_printf(s, ",snapshot=%llu", tcon->snapshot_time);
> > >         if (tcon->handle_timeout)
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index f873379066c7..e28a23b617ef 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -679,6 +679,7 @@ struct TCP_Server_Info {
> > >         struct smbd_connection *smbd_conn;
> > >         struct delayed_work     echo; /* echo ping workqueue job */
> > >         struct delayed_work     resolve; /* dns resolution workqueue 
> > > job */
> > > +       unsigned int dns_interval; /* interval for resolve worker */
> > >         char    *smallbuf;      /* pointer to current "small" buffer */
> > >         char    *bigbuf;        /* pointer to current "big" buffer */
> > >         /* Total size of this PDU. Only valid from 
> > > cifs_demultiplex_thread */
> > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > index 06bafba9c3ff..e6bedced576a 100644
> > > --- a/fs/cifs/connect.c
> > > +++ b/fs/cifs/connect.c
> > > @@ -92,7 +92,7 @@ static int reconn_set_ipaddr_from_hostname(struct 
> > > TCP_Server_Info *server)
> > >         int len;
> > >         char *unc, *ipaddr = NULL;
> > >         time64_t expiry, now;
> > > -       unsigned long ttl = SMB_DNS_RESOLVE_INTERVAL_DEFAULT;
> > > +       unsigned int ttl = server->dns_interval;
> > >
> > >         if (!server->hostname ||
> > >             server->hostname[0] == '\0')
> > > @@ -129,13 +129,15 @@ static int 
> > > reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
> > >                         /*
> > >                          * To make sure we don't use the cached 
> > > entry, retry 1s
> > >                          * after expiry.
> > > +                        *
> > > +                        * dns_interval is guaranteed to be >= 
> > > SMB_DNS_RESOLVE_INTERVAL_MIN
> > >                          */
> > > -                       ttl = max_t(unsigned long, expiry - now, 
> > > SMB_DNS_RESOLVE_INTERVAL_MIN) + 1;
> > > +                       ttl = max_t(unsigned long, expiry - now, 
> > > server->dns_interval) + 1;
> > >         }
> > >         rc = !rc ? -1 : 0;
> > >
> > >  requeue_resolve:
> > > -       cifs_dbg(FYI, "%s: next dns resolution scheduled for %lu 
> > > seconds in the future\n",
> > > +       cifs_dbg(FYI, "%s: next dns resolution scheduled for %u 
> > > seconds in the future\n",
> > >                  __func__, ttl);
> > >         mod_delayed_work(cifsiod_wq, &server->resolve, (ttl * HZ));
> > >
> > > @@ -1608,6 +1610,12 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
> > >                 tcp_ses->echo_interval = ctx->echo_interval * HZ;
> > >         else
> > >                 tcp_ses->echo_interval = SMB_ECHO_INTERVAL_DEFAULT * HZ;
> > > +
> > > +       if (ctx->dns_interval >= SMB_DNS_RESOLVE_INTERVAL_MIN)
> > > +               tcp_ses->dns_interval = ctx->dns_interval;
> > > +       else
> > > +               tcp_ses->dns_interval = 
> > > SMB_DNS_RESOLVE_INTERVAL_DEFAULT;
> > > +
> > Hi Enzo,
> >
> > Is the above line a mistake? Shouldn't that be set to
> > SMB_DNS_RESOLVE_INTERVAL_MIN value in the else case?
> > Rest looks good to me. You can add my RB.
>
> Hy Shyam,
>
> SMB_DNS_RESOLVE_INTERVAL_MIN is just for boundary-checking. In case
> dns_interval is < than that, we fallback to the default value (I've copied
> echo_interval behaviour, and also the current behaviour).
>
> IMHO we shouldn't use SMB_DNS_RESOLVE_INTERVAL_MIN as a fallback value
> because it's too far from the default values used by the servers I've
> checked (Windows Server 2019, 600s, samba "name cache timeout" = 660s).
>
>
> Thanks,
>
> Enzo