Re: [PATCH] cifs: introduce dns_interval mount option

[Tom Talpey <tom@talpey.com>]

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@talpey.com>

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@suse.de> 
>> 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@suse.de>
>>> ---
>>>  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
>