All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Introduce dns_interval procfs setting
@ 2022-06-08 21:54 Enzo Matsumiya
  2022-06-08 21:54 ` [PATCH 1/2] cifs: create procfs for dns_interval setting Enzo Matsumiya
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-08 21:54 UTC (permalink / raw)
  To: linux-cifs; +Cc: smfrench, pc, ronniesahlberg, nspmangalore, Enzo Matsumiya

Hi,

These 2 patches are a simple way to fix the DNS issue that
currently exists in cifs, where the upcall to key.dns_resolver will
always return 0 for the record TTL, hence, making the resolve worker
always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
(currently 600 seconds).

This also makes the new setting `dns_interval' user-configurable via
procfs (/proc/fs/cifs/dns_interval).

One disadvantage here is that the interval is applied to all hosts
resolution. This is still how it works today, because we're always using
the default value anyway, but should someday this be fixed, then we can
go back to rely on the keys infrastructure to cache each hostname with
its own separate TTL.

Please review and test. All feedback is welcome.


Cheers

Enzo

Enzo Matsumiya (2):
  cifs: create procfs for dns_interval setting
  cifs: reschedule DNS resolve worker based on dns_interval

 fs/cifs/cifs_debug.c  | 63 +++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/cifs_debug.h  |  2 ++
 fs/cifs/cifsfs.c      |  1 +
 fs/cifs/connect.c     |  4 +--
 fs/cifs/dns_resolve.c |  8 ++++++
 5 files changed, 76 insertions(+), 2 deletions(-)

-- 
2.36.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] cifs: create procfs for dns_interval setting
  2022-06-08 21:54 [PATCH 0/2] Introduce dns_interval procfs setting Enzo Matsumiya
@ 2022-06-08 21:54 ` Enzo Matsumiya
  2022-06-08 21:54 ` [PATCH 2/2] cifs: reschedule DNS resolve worker based on dns_interval Enzo Matsumiya
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-08 21:54 UTC (permalink / raw)
  To: linux-cifs; +Cc: smfrench, pc, ronniesahlberg, nspmangalore, Enzo Matsumiya

This patch introduces the /proc/fs/cifs/dns_interval setting, used to
configure the interval that DNS resolutions must be done.

Enforces the minimum value SMB_DNS_RESOLVE_INTERVAL_MIN (currently 120),
but allows it to be lower (10, arbitrarily chosen) when debugging (i.e.
cifsFYI > 0).

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/cifs_debug.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/cifs_debug.h |  2 ++
 fs/cifs/cifsfs.c     |  1 +
 3 files changed, 66 insertions(+)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 1dd995efd5b8..96ff549a103e 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -695,6 +695,7 @@ PROC_FILE_DEFINE(smbd_receive_credit_max);
 
 static struct proc_dir_entry *proc_fs_cifs;
 static const struct proc_ops cifsFYI_proc_ops;
+static const struct proc_ops cifs_dns_interval_proc_ops;
 static const struct proc_ops cifs_lookup_cache_proc_ops;
 static const struct proc_ops traceSMB_proc_ops;
 static const struct proc_ops cifs_security_flags_proc_ops;
@@ -716,6 +717,7 @@ cifs_proc_init(void)
 
 	proc_create("Stats", 0644, proc_fs_cifs, &cifs_stats_proc_ops);
 	proc_create("cifsFYI", 0644, proc_fs_cifs, &cifsFYI_proc_ops);
+	proc_create("dns_interval", 0644, proc_fs_cifs, &cifs_dns_interval_proc_ops);
 	proc_create("traceSMB", 0644, proc_fs_cifs, &traceSMB_proc_ops);
 	proc_create("LinuxExtensionsEnabled", 0644, proc_fs_cifs,
 		    &cifs_linux_ext_proc_ops);
@@ -759,6 +761,7 @@ cifs_proc_clean(void)
 	remove_proc_entry("DebugData", proc_fs_cifs);
 	remove_proc_entry("open_files", proc_fs_cifs);
 	remove_proc_entry("cifsFYI", proc_fs_cifs);
+	remove_proc_entry("dns_interval", proc_fs_cifs);
 	remove_proc_entry("traceSMB", proc_fs_cifs);
 	remove_proc_entry("Stats", proc_fs_cifs);
 	remove_proc_entry("SecurityFlags", proc_fs_cifs);
@@ -821,6 +824,66 @@ static const struct proc_ops cifsFYI_proc_ops = {
 	.proc_write	= cifsFYI_proc_write,
 };
 
+static int cifs_dns_interval_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "%u\n", dns_interval);
+	return 0;
+}
+
+static int cifs_dns_interval_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, cifs_dns_interval_show, NULL);
+}
+
+static ssize_t cifs_dns_interval_write(struct file *file, const char __user *buffer,
+		size_t count, loff_t *ppos)
+{
+	int rc;
+	unsigned int interval;
+	char buf[12] = { 0 };
+	/* allow the minimum interval to be 10 (arbritrary) when debugging */
+	unsigned int min_interval = cifsFYI ? 10 : SMB_DNS_RESOLVE_INTERVAL_MIN;
+
+	if ((count < 1) || (count > 11))
+		return -EINVAL;
+
+	if (copy_from_user(buf, buffer, count))
+		return -EFAULT;
+
+	if (count < 3) {
+		if (!isdigit(buf[0])) {
+			cifs_dbg(VFS, "Invalid value for dns_interval: %s\n",
+					buf);
+			return -EINVAL;
+		}
+	}
+
+	rc = kstrtouint(buf, 0, &interval);
+	if (rc) {
+		cifs_dbg(VFS, "Invalid value for dns_interval: %s\n",
+				buf);
+		return rc;
+	}
+
+	if (interval < min_interval) {
+		cifs_dbg(VFS, "minimum value for dns_interval is %u, default value is %u\n",
+				SMB_DNS_RESOLVE_INTERVAL_MIN,
+				SMB_DNS_RESOLVE_INTERVAL_DEFAULT);
+		return -EINVAL;
+	}
+
+	dns_interval = interval;
+	return count;
+}
+
+static const struct proc_ops cifs_dns_interval_proc_ops = {
+	.proc_open	= cifs_dns_interval_open,
+	.proc_read	= seq_read,
+	.proc_lseek	= seq_lseek,
+	.proc_release	= single_release,
+	.proc_write	= cifs_dns_interval_write,
+};
+
 static int cifs_linux_ext_proc_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "%d\n", linuxExtEnabled);
diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index ee4ea2b60c0f..40ef322450a3 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -33,6 +33,8 @@ extern int cifsFYI;
 #endif
 #define ONCE 8
 
+extern unsigned int dns_interval;
+
 /*
  *	debug ON
  *	--------
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 325423180fd2..284645da8cd3 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -66,6 +66,7 @@ bool enable_gcm_256 = true;
 bool require_gcm_256; /* false by default */
 bool enable_negotiate_signing; /* false by default */
 unsigned int global_secflags = CIFSSEC_DEF;
+unsigned int dns_interval = SMB_DNS_RESOLVE_INTERVAL_DEFAULT;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
 static const struct super_operations cifs_super_ops;
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] cifs: reschedule DNS resolve worker based on dns_interval
  2022-06-08 21:54 [PATCH 0/2] Introduce dns_interval procfs setting Enzo Matsumiya
  2022-06-08 21:54 ` [PATCH 1/2] cifs: create procfs for dns_interval setting Enzo Matsumiya
@ 2022-06-08 21:54 ` Enzo Matsumiya
  2022-06-09  0:39 ` [PATCH 0/2] Introduce dns_interval procfs setting ronnie sahlberg
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-08 21:54 UTC (permalink / raw)
  To: linux-cifs; +Cc: smfrench, pc, ronniesahlberg, nspmangalore, Enzo Matsumiya

This patch makes use of the dns_interval introduced in the previous
commit when re/scheduling the resolve worker.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/connect.c     | 4 ++--
 fs/cifs/dns_resolve.c | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 06bafba9c3ff..edd4f0020f9c 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 long ttl = dns_interval;
 
 	if (!server->hostname ||
 	    server->hostname[0] == '\0')
@@ -130,7 +130,7 @@ 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.
 			 */
-			ttl = max_t(unsigned long, expiry - now, SMB_DNS_RESOLVE_INTERVAL_MIN) + 1;
+			ttl = max_t(unsigned long, expiry - now, dns_interval) + 1;
 	}
 	rc = !rc ? -1 : 0;
 
diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
index 0458d28d71aa..eff1e61180ed 100644
--- a/fs/cifs/dns_resolve.c
+++ b/fs/cifs/dns_resolve.c
@@ -67,6 +67,14 @@ dns_resolve_server_name_to_ip(const char *unc, char **ip_addr, time64_t *expiry)
 	/* Perform the upcall */
 	rc = dns_query(current->nsproxy->net_ns, NULL, hostname, len,
 		       NULL, ip_addr, expiry, false);
+
+	/*
+	 * If the upcall didn't get a TTL, we use the value from dns_interval
+	 * procfs setting
+	 */
+	if (expiry && *expiry == 0)
+		*expiry = dns_interval;
+
 	if (rc < 0)
 		cifs_dbg(FYI, "%s: unable to resolve: %*.*s\n",
 			 __func__, len, len, hostname);
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-08 21:54 [PATCH 0/2] Introduce dns_interval procfs setting Enzo Matsumiya
  2022-06-08 21:54 ` [PATCH 1/2] cifs: create procfs for dns_interval setting Enzo Matsumiya
  2022-06-08 21:54 ` [PATCH 2/2] cifs: reschedule DNS resolve worker based on dns_interval Enzo Matsumiya
@ 2022-06-09  0:39 ` ronnie sahlberg
  2022-06-09 14:17 ` Tom Talpey
  2022-06-09 14:53 ` Paulo Alcantara
  4 siblings, 0 replies; 15+ messages in thread
From: ronnie sahlberg @ 2022-06-09  0:39 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: linux-cifs, Steve French, Paulo Alcantara, Shyam Prasad N

Looks good. Reviewed-by for both patches.

On Thu, 9 Jun 2022 at 07:55, Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Hi,
>
> These 2 patches are a simple way to fix the DNS issue that
> currently exists in cifs, where the upcall to key.dns_resolver will
> always return 0 for the record TTL, hence, making the resolve worker
> always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
> (currently 600 seconds).
>
> This also makes the new setting `dns_interval' user-configurable via
> procfs (/proc/fs/cifs/dns_interval).
>
> One disadvantage here is that the interval is applied to all hosts
> resolution. This is still how it works today, because we're always using
> the default value anyway, but should someday this be fixed, then we can
> go back to rely on the keys infrastructure to cache each hostname with
> its own separate TTL.
>
> Please review and test. All feedback is welcome.
>
>
> Cheers
>
> Enzo
>
> Enzo Matsumiya (2):
>   cifs: create procfs for dns_interval setting
>   cifs: reschedule DNS resolve worker based on dns_interval
>
>  fs/cifs/cifs_debug.c  | 63 +++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/cifs_debug.h  |  2 ++
>  fs/cifs/cifsfs.c      |  1 +
>  fs/cifs/connect.c     |  4 +--
>  fs/cifs/dns_resolve.c |  8 ++++++
>  5 files changed, 76 insertions(+), 2 deletions(-)
>
> --
> 2.36.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-08 21:54 [PATCH 0/2] Introduce dns_interval procfs setting Enzo Matsumiya
                   ` (2 preceding siblings ...)
  2022-06-09  0:39 ` [PATCH 0/2] Introduce dns_interval procfs setting ronnie sahlberg
@ 2022-06-09 14:17 ` Tom Talpey
  2022-06-09 15:03   ` Enzo Matsumiya
  2022-06-09 14:53 ` Paulo Alcantara
  4 siblings, 1 reply; 15+ messages in thread
From: Tom Talpey @ 2022-06-09 14:17 UTC (permalink / raw)
  To: Enzo Matsumiya, linux-cifs; +Cc: smfrench, pc, ronniesahlberg, nspmangalore

On 6/8/2022 5:54 PM, Enzo Matsumiya wrote:
> Hi,
> 
> These 2 patches are a simple way to fix the DNS issue that
> currently exists in cifs, where the upcall to key.dns_resolver will
> always return 0 for the record TTL, hence, making the resolve worker
> always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
> (currently 600 seconds).
> 
> This also makes the new setting `dns_interval' user-configurable via
> procfs (/proc/fs/cifs/dns_interval).
> 
> One disadvantage here is that the interval is applied to all hosts
> resolution. This is still how it works today, because we're always using
> the default value anyway, but should someday this be fixed, then we can
> go back to rely on the keys infrastructure to cache each hostname with
> its own separate TTL.

Curious, why did you choose a procfs global approach? Wouldn't it be
more appropriate to make this a mount option, so it would be applied
selectively per-server?

Tom.

> Please review and test. All feedback is welcome.
> 
> 
> Cheers
> 
> Enzo
> 
> Enzo Matsumiya (2):
>    cifs: create procfs for dns_interval setting
>    cifs: reschedule DNS resolve worker based on dns_interval
> 
>   fs/cifs/cifs_debug.c  | 63 +++++++++++++++++++++++++++++++++++++++++++
>   fs/cifs/cifs_debug.h  |  2 ++
>   fs/cifs/cifsfs.c      |  1 +
>   fs/cifs/connect.c     |  4 +--
>   fs/cifs/dns_resolve.c |  8 ++++++
>   5 files changed, 76 insertions(+), 2 deletions(-)
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-08 21:54 [PATCH 0/2] Introduce dns_interval procfs setting Enzo Matsumiya
                   ` (3 preceding siblings ...)
  2022-06-09 14:17 ` Tom Talpey
@ 2022-06-09 14:53 ` Paulo Alcantara
  2022-06-09 15:14   ` Enzo Matsumiya
  4 siblings, 1 reply; 15+ messages in thread
From: Paulo Alcantara @ 2022-06-09 14:53 UTC (permalink / raw)
  To: Enzo Matsumiya, linux-cifs
  Cc: smfrench, ronniesahlberg, nspmangalore, Enzo Matsumiya

Hi Enzo,

Enzo Matsumiya <ematsumiya@suse.de> writes:

> These 2 patches are a simple way to fix the DNS issue that
> currently exists in cifs, where the upcall to key.dns_resolver will
> always return 0 for the record TTL, hence, making the resolve worker
> always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
> (currently 600 seconds).
>
> This also makes the new setting `dns_interval' user-configurable via
> procfs (/proc/fs/cifs/dns_interval).

How is the user supposed to know which TTL value will be used?  If the
expire time is set by either key.dns_resolver or any other program used
for dns_resolver key, the above setting would no longer work and users
might get confused.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 14:17 ` Tom Talpey
@ 2022-06-09 15:03   ` Enzo Matsumiya
  2022-06-09 15:24     ` Tom Talpey
  0 siblings, 1 reply; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-09 15:03 UTC (permalink / raw)
  To: Tom Talpey; +Cc: linux-cifs, smfrench, pc, ronniesahlberg, nspmangalore

On 06/09, Tom Talpey wrote:
>On 6/8/2022 5:54 PM, Enzo Matsumiya wrote:
>>Hi,
>>
>>These 2 patches are a simple way to fix the DNS issue that
>>currently exists in cifs, where the upcall to key.dns_resolver will
>>always return 0 for the record TTL, hence, making the resolve worker
>>always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
>>(currently 600 seconds).
>>
>>This also makes the new setting `dns_interval' user-configurable via
>>procfs (/proc/fs/cifs/dns_interval).
>>
>>One disadvantage here is that the interval is applied to all hosts
>>resolution. This is still how it works today, because we're always using
>>the default value anyway, but should someday this be fixed, then we can
>>go back to rely on the keys infrastructure to cache each hostname with
>>its own separate TTL.
>
>Curious, why did you choose a procfs global approach? Wouldn't it be
>more appropriate to make this a mount option, so it would be applied
>selectively per-server?

I tried to stick to the current behaviour, while also fixing the issue
of getting a TTL=0 from key.dns_resolver upcall.

A mount option looks indeed better initially, and that was also my
initial approach to this. But in a, e.g. multi-domain forest, with
multiple DFS targets, the per-mount setting starts to look irrelevant
again as well. So I took the "per-client" approach instead.


Cheers,

Enzo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 14:53 ` Paulo Alcantara
@ 2022-06-09 15:14   ` Enzo Matsumiya
  2022-06-09 15:21     ` Paulo Alcantara
  0 siblings, 1 reply; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-09 15:14 UTC (permalink / raw)
  To: Paulo Alcantara; +Cc: linux-cifs, smfrench, ronniesahlberg, nspmangalore

Hi Paulo,

On 06/09, Paulo Alcantara wrote:
>Hi Enzo,
>
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> These 2 patches are a simple way to fix the DNS issue that
>> currently exists in cifs, where the upcall to key.dns_resolver will
>> always return 0 for the record TTL, hence, making the resolve worker
>> always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
>> (currently 600 seconds).
>>
>> This also makes the new setting `dns_interval' user-configurable via
>> procfs (/proc/fs/cifs/dns_interval).
>
>How is the user supposed to know which TTL value will be used?  If the
>expire time is set by either key.dns_resolver or any other program used
>for dns_resolver key, the above setting would no longer work and users
>might get confused.

The initial value is the same as before (SMB_DNS_RESOLVE_INTERVAL_DEFAULT, 600s).

The user don't need to know a value to be used, unless they know the
value the server uses (either manually set by themselves or by some
external script) and then they can use that same value for this new
dns_interval setting.

A very simple example on how one could do it, if there's no desire to
use the default 600s:

# TTL=$(dig +noall +answer my.server.domain | awk '{ print $2 }')
# echo $TTL > /proc/fs/cifs/dns_interval


Cheers,

Enzo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 15:14   ` Enzo Matsumiya
@ 2022-06-09 15:21     ` Paulo Alcantara
  2022-06-09 15:30       ` Enzo Matsumiya
  0 siblings, 1 reply; 15+ messages in thread
From: Paulo Alcantara @ 2022-06-09 15:21 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: linux-cifs, smfrench, ronniesahlberg, nspmangalore

Enzo Matsumiya <ematsumiya@suse.de> writes:

> The initial value is the same as before (SMB_DNS_RESOLVE_INTERVAL_DEFAULT, 600s).
>
> The user don't need to know a value to be used, unless they know the
> value the server uses (either manually set by themselves or by some
> external script) and then they can use that same value for this new
> dns_interval setting.
>
> A very simple example on how one could do it, if there's no desire to
> use the default 600s:
>
> # TTL=$(dig +noall +answer my.server.domain | awk '{ print $2 }')
> # echo $TTL > /proc/fs/cifs/dns_interval

That's not what I meant.  Sorry if I was unclear.

If the upcalled program sets the record TTL (key's expire time), then
any values set in /proc/fs/cifs/dns_interval will not work.  The user
might expect it to work, though.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 15:03   ` Enzo Matsumiya
@ 2022-06-09 15:24     ` Tom Talpey
  2022-06-09 16:17       ` Enzo Matsumiya
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Talpey @ 2022-06-09 15:24 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: linux-cifs, smfrench, pc, ronniesahlberg, nspmangalore

On 6/9/2022 11:03 AM, Enzo Matsumiya wrote:
> On 06/09, Tom Talpey wrote:
>> On 6/8/2022 5:54 PM, Enzo Matsumiya wrote:
>>> Hi,
>>>
>>> These 2 patches are a simple way to fix the DNS issue that
>>> currently exists in cifs, where the upcall to key.dns_resolver will
>>> always return 0 for the record TTL, hence, making the resolve worker
>>> always use the default value SMB_DNS_RESOLVE_INTERVAL_DEFAULT
>>> (currently 600 seconds).
>>>
>>> This also makes the new setting `dns_interval' user-configurable via
>>> procfs (/proc/fs/cifs/dns_interval).
>>>
>>> One disadvantage here is that the interval is applied to all hosts
>>> resolution. This is still how it works today, because we're always using
>>> the default value anyway, but should someday this be fixed, then we can
>>> go back to rely on the keys infrastructure to cache each hostname with
>>> its own separate TTL.
>>
>> Curious, why did you choose a procfs global approach? Wouldn't it be
>> more appropriate to make this a mount option, so it would be applied
>> selectively per-server?
> 
> I tried to stick to the current behaviour, while also fixing the issue
> of getting a TTL=0 from key.dns_resolver upcall.
> 
> A mount option looks indeed better initially, and that was also my
> initial approach to this. But in a, e.g. multi-domain forest, with
> multiple DFS targets, the per-mount setting starts to look irrelevant
> again as well. So I took the "per-client" approach instead.

Ok, I guess. It seems like kicking the can down the road, a little.
I agree that rearchitecting the DNS cache might be extreme, but many
distros consider procfs to be a user API, and they'll require it to
be supported basically forever. That would be unfortunate...

Tom.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 15:21     ` Paulo Alcantara
@ 2022-06-09 15:30       ` Enzo Matsumiya
  2022-06-09 15:49         ` Paulo Alcantara
  0 siblings, 1 reply; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-09 15:30 UTC (permalink / raw)
  To: Paulo Alcantara; +Cc: linux-cifs, smfrench, ronniesahlberg, nspmangalore

On 06/09, Paulo Alcantara wrote:
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> The initial value is the same as before (SMB_DNS_RESOLVE_INTERVAL_DEFAULT, 600s).
>>
>> The user don't need to know a value to be used, unless they know the
>> value the server uses (either manually set by themselves or by some
>> external script) and then they can use that same value for this new
>> dns_interval setting.
>>
>> A very simple example on how one could do it, if there's no desire to
>> use the default 600s:
>>
>> # TTL=$(dig +noall +answer my.server.domain | awk '{ print $2 }')
>> # echo $TTL > /proc/fs/cifs/dns_interval
>
>That's not what I meant.  Sorry if I was unclear.
>
>If the upcalled program sets the record TTL (key's expire time), then
>any values set in /proc/fs/cifs/dns_interval will not work.  The user
>might expect it to work, though.

Exactly.

Currently, key.dns_resolver uses getaddrinfo() to resolve names, which
doesn't contain the TTL for the record, hence *always* returns 0 to cifs.ko.
This patch is just a way to provide some flexibility to the user, in
case they don't want to use the currently-always-fixed 600s.

As we discussed, this is supposed to be fixed in key.dns_resolver, but
my proposed patch for that never got ack'd, or even replied to. I might
try again at some point.


Cheers,

Enzo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 15:30       ` Enzo Matsumiya
@ 2022-06-09 15:49         ` Paulo Alcantara
  2022-06-09 16:16           ` Enzo Matsumiya
  0 siblings, 1 reply; 15+ messages in thread
From: Paulo Alcantara @ 2022-06-09 15:49 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: linux-cifs, smfrench, ronniesahlberg, nspmangalore

Enzo Matsumiya <ematsumiya@suse.de> writes:

> Currently, key.dns_resolver uses getaddrinfo() to resolve names, which
> doesn't contain the TTL for the record, hence *always* returns 0 to cifs.ko.
> This patch is just a way to provide some flexibility to the user, in
> case they don't want to use the currently-always-fixed 600s.

It is not limited to key.dns_resolver.  The user is free to choose
whatever program he/she wants to be upcalled for dns_resolver key.

For instance, some distros might still be using cifs.upcall(8) that
actually set record TTL, thus making it impossible for the user to
change default via /proc/fs/cifs/dns_interval.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 15:49         ` Paulo Alcantara
@ 2022-06-09 16:16           ` Enzo Matsumiya
  2022-06-09 16:42             ` Paulo Alcantara
  0 siblings, 1 reply; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-09 16:16 UTC (permalink / raw)
  To: Paulo Alcantara; +Cc: linux-cifs, smfrench, ronniesahlberg, nspmangalore

On 06/09, Paulo Alcantara wrote:
>Enzo Matsumiya <ematsumiya@suse.de> writes:
>
>> Currently, key.dns_resolver uses getaddrinfo() to resolve names, which
>> doesn't contain the TTL for the record, hence *always* returns 0 to cifs.ko.
>> This patch is just a way to provide some flexibility to the user, in
>> case they don't want to use the currently-always-fixed 600s.
>
>It is not limited to key.dns_resolver.  The user is free to choose
>whatever program he/she wants to be upcalled for dns_resolver key.
>
>For instance, some distros might still be using cifs.upcall(8) that
>actually set record TTL, thus making it impossible for the user to
>change default via /proc/fs/cifs/dns_interval.

Ah sorry, I misunderstood.

But this patch isn't supposed to allow the user to change the _default_ TTL
value, but only to give them a chance to change the TTL value *iff* the
upcall returned 0.

In case the upcall returns TTL != 0, dns_resolve_server_name_to_ip()
will use that value instead, which, again, maintains the current behaviour.

But yes, if desired, I can adjust the patch to completely ignore the
TTL value from upcall and manage it by ourselves always, either by
procfs or by mount option.

What do you think?


Cheers,

Enzo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 15:24     ` Tom Talpey
@ 2022-06-09 16:17       ` Enzo Matsumiya
  0 siblings, 0 replies; 15+ messages in thread
From: Enzo Matsumiya @ 2022-06-09 16:17 UTC (permalink / raw)
  To: Tom Talpey; +Cc: linux-cifs, smfrench, pc, ronniesahlberg, nspmangalore

On 06/09, Tom Talpey wrote:
>>I tried to stick to the current behaviour, while also fixing the issue
>>of getting a TTL=0 from key.dns_resolver upcall.
>>
>>A mount option looks indeed better initially, and that was also my
>>initial approach to this. But in a, e.g. multi-domain forest, with
>>multiple DFS targets, the per-mount setting starts to look irrelevant
>>again as well. So I took the "per-client" approach instead.
>
>Ok, I guess. It seems like kicking the can down the road, a little.
>I agree that rearchitecting the DNS cache might be extreme, but many
>distros consider procfs to be a user API, and they'll require it to
>be supported basically forever. That would be unfortunate...

Thanks, I didn't know that. I'll re-work on the patch and take this in
consideration then.


Cheers,

Enzo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/2] Introduce dns_interval procfs setting
  2022-06-09 16:16           ` Enzo Matsumiya
@ 2022-06-09 16:42             ` Paulo Alcantara
  0 siblings, 0 replies; 15+ messages in thread
From: Paulo Alcantara @ 2022-06-09 16:42 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: linux-cifs, smfrench, ronniesahlberg, nspmangalore

Enzo Matsumiya <ematsumiya@suse.de> writes:

> On 06/09, Paulo Alcantara wrote:
>>Enzo Matsumiya <ematsumiya@suse.de> writes:
>>
>>> Currently, key.dns_resolver uses getaddrinfo() to resolve names, which
>>> doesn't contain the TTL for the record, hence *always* returns 0 to cifs.ko.
>>> This patch is just a way to provide some flexibility to the user, in
>>> case they don't want to use the currently-always-fixed 600s.
>>
>>It is not limited to key.dns_resolver.  The user is free to choose
>>whatever program he/she wants to be upcalled for dns_resolver key.
>>
>>For instance, some distros might still be using cifs.upcall(8) that
>>actually set record TTL, thus making it impossible for the user to
>>change default via /proc/fs/cifs/dns_interval.
>
> Ah sorry, I misunderstood.
>
> But this patch isn't supposed to allow the user to change the _default_ TTL
> value, but only to give them a chance to change the TTL value *iff* the
> upcall returned 0.

That was my concern.  If we expose it to users, they would probably
expect it work at all times regardless whether the key's expire time was
set or not.

> In case the upcall returns TTL != 0, dns_resolve_server_name_to_ip()
> will use that value instead, which, again, maintains the current behaviour.

Then it would start ignoring values from /proc/fs/cifs/dns_interval and
not telling the user why.

> But yes, if desired, I can adjust the patch to completely ignore the
> TTL value from upcall and manage it by ourselves always, either by
> procfs or by mount option.

That would work, too.  BTW, I'd personally go with the mount option
version.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-06-09 16:42 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-08 21:54 [PATCH 0/2] Introduce dns_interval procfs setting Enzo Matsumiya
2022-06-08 21:54 ` [PATCH 1/2] cifs: create procfs for dns_interval setting Enzo Matsumiya
2022-06-08 21:54 ` [PATCH 2/2] cifs: reschedule DNS resolve worker based on dns_interval Enzo Matsumiya
2022-06-09  0:39 ` [PATCH 0/2] Introduce dns_interval procfs setting ronnie sahlberg
2022-06-09 14:17 ` Tom Talpey
2022-06-09 15:03   ` Enzo Matsumiya
2022-06-09 15:24     ` Tom Talpey
2022-06-09 16:17       ` Enzo Matsumiya
2022-06-09 14:53 ` Paulo Alcantara
2022-06-09 15:14   ` Enzo Matsumiya
2022-06-09 15:21     ` Paulo Alcantara
2022-06-09 15:30       ` Enzo Matsumiya
2022-06-09 15:49         ` Paulo Alcantara
2022-06-09 16:16           ` Enzo Matsumiya
2022-06-09 16:42             ` Paulo Alcantara

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.