| Message ID | 20250213154722.37499-1-okorniev@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/1] nfs-utils: nfsdctl: dont ignore rdma listener return | expand |
On 2/13/25 10:47 AM, Olga Kornievskaia wrote: > Don't ignore return code of adding rdma listener. If nfs.conf has asked > for "rdma=y" but adding the listener fails, don't ignore the failure. > Note in soft-rdma-provider environment (such as soft iwarp, soft roce), > when no address-constraints are used, an "any" listener is created and > rdma-enabling is done independently. This behavior is confusing... I suggest that an nfs.conf man page update accompany the below code change. Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > --- > utils/nfsdctl/nfsdctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > index 05fecc71..244910ef 100644 > --- a/utils/nfsdctl/nfsdctl.c > +++ b/utils/nfsdctl/nfsdctl.c > @@ -1388,7 +1388,7 @@ static int configure_listeners(void) > if (tcp) > ret = add_listener("tcp", n->field, port); > if (rdma) > - add_listener("rdma", n->field, rdma_port); > + ret = add_listener("rdma", n->field, rdma_port); > if (ret) > return ret; > } > @@ -1398,7 +1398,7 @@ static int configure_listeners(void) > if (tcp) > ret = add_listener("tcp", "", port); > if (rdma) > - add_listener("rdma", "", rdma_port); > + ret = add_listener("rdma", "", rdma_port); > } > return ret; > }
On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 2/13/25 10:47 AM, Olga Kornievskaia wrote: > > Don't ignore return code of adding rdma listener. If nfs.conf has asked > > for "rdma=y" but adding the listener fails, don't ignore the failure. > > Note in soft-rdma-provider environment (such as soft iwarp, soft roce), > > when no address-constraints are used, an "any" listener is created and > > rdma-enabling is done independently. > > This behavior is confusing... I suggest that an nfs.conf man page > update accompany the below code change. Do you find only the rdma=y soft-rdma case confusing, or do you find that when listeners fail and we shouldn't start knfsd threads in general confusing? It was always the case that if rdma=y is done, then any listener created for it does not check whether or not the underlying interface is already rdma-enabled. This hasn't changed. Nor does this patch change it. > Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > > > > Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > > --- > > utils/nfsdctl/nfsdctl.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > > index 05fecc71..244910ef 100644 > > --- a/utils/nfsdctl/nfsdctl.c > > +++ b/utils/nfsdctl/nfsdctl.c > > @@ -1388,7 +1388,7 @@ static int configure_listeners(void) > > if (tcp) > > ret = add_listener("tcp", n->field, port); > > if (rdma) > > - add_listener("rdma", n->field, rdma_port); > > + ret = add_listener("rdma", n->field, rdma_port); > > if (ret) > > return ret; > > } > > @@ -1398,7 +1398,7 @@ static int configure_listeners(void) > > if (tcp) > > ret = add_listener("tcp", "", port); > > if (rdma) > > - add_listener("rdma", "", rdma_port); > > + ret = add_listener("rdma", "", rdma_port); > > } > > return ret; > > } > > > -- > Chuck Lever >
On 2/13/25 12:30 PM, Olga Kornievskaia wrote: > On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On 2/13/25 10:47 AM, Olga Kornievskaia wrote: >>> Don't ignore return code of adding rdma listener. If nfs.conf has asked >>> for "rdma=y" but adding the listener fails, don't ignore the failure. >>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce), >>> when no address-constraints are used, an "any" listener is created and >>> rdma-enabling is done independently. >> >> This behavior is confusing... I suggest that an nfs.conf man page >> update accompany the below code change. > > Do you find only the rdma=y soft-rdma case confusing, or do you find > that when listeners fail and we shouldn't start knfsd threads in > general confusing? > > It was always the case that if rdma=y is done, then any listener > created for it does not check whether or not the underlying interface > is already rdma-enabled. This hasn't changed. Nor does this patch > change it. Not saying the patch changes the behavior. But you have to admit the behavior is surprising and needs clear documentation. >> Reviewed-by: Chuck Lever <chuck.lever@oracle.com> >> >> >>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >>> --- >>> utils/nfsdctl/nfsdctl.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c >>> index 05fecc71..244910ef 100644 >>> --- a/utils/nfsdctl/nfsdctl.c >>> +++ b/utils/nfsdctl/nfsdctl.c >>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void) >>> if (tcp) >>> ret = add_listener("tcp", n->field, port); >>> if (rdma) >>> - add_listener("rdma", n->field, rdma_port); >>> + ret = add_listener("rdma", n->field, rdma_port); >>> if (ret) >>> return ret; >>> } >>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void) >>> if (tcp) >>> ret = add_listener("tcp", "", port); >>> if (rdma) >>> - add_listener("rdma", "", rdma_port); >>> + ret = add_listener("rdma", "", rdma_port); >>> } >>> return ret; >>> } >> >> >> -- >> Chuck Lever >> >
On Fri, Feb 14, 2025 at 9:24 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 2/13/25 12:30 PM, Olga Kornievskaia wrote: > > On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote: > >> > >> On 2/13/25 10:47 AM, Olga Kornievskaia wrote: > >>> Don't ignore return code of adding rdma listener. If nfs.conf has asked > >>> for "rdma=y" but adding the listener fails, don't ignore the failure. > >>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce), > >>> when no address-constraints are used, an "any" listener is created and > >>> rdma-enabling is done independently. > >> > >> This behavior is confusing... I suggest that an nfs.conf man page > >> update accompany the below code change. > > > > Do you find only the rdma=y soft-rdma case confusing, or do you find > > that when listeners fail and we shouldn't start knfsd threads in > > general confusing? > > > > It was always the case that if rdma=y is done, then any listener > > created for it does not check whether or not the underlying interface > > is already rdma-enabled. This hasn't changed. Nor does this patch > > change it. > > Not saying the patch changes the behavior. But you have to admit the > behavior is surprising and needs clear documentation. Sure we can document the behavior of the any listener on a soft rdma interface as it's used by the knfsd. But is it guaranteed not to change, as the behaviour is controlled by the RDMA core not NFS? > >> Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > >> > >> > >>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") > >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > >>> --- > >>> utils/nfsdctl/nfsdctl.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > >>> index 05fecc71..244910ef 100644 > >>> --- a/utils/nfsdctl/nfsdctl.c > >>> +++ b/utils/nfsdctl/nfsdctl.c > >>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void) > >>> if (tcp) > >>> ret = add_listener("tcp", n->field, port); > >>> if (rdma) > >>> - add_listener("rdma", n->field, rdma_port); > >>> + ret = add_listener("rdma", n->field, rdma_port); > >>> if (ret) > >>> return ret; > >>> } > >>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void) > >>> if (tcp) > >>> ret = add_listener("tcp", "", port); > >>> if (rdma) > >>> - add_listener("rdma", "", rdma_port); > >>> + ret = add_listener("rdma", "", rdma_port); > >>> } > >>> return ret; > >>> } > >> > >> > >> -- > >> Chuck Lever > >> > > > > > -- > Chuck Lever >
On 2/14/25 10:38 AM, Olga Kornievskaia wrote: > On Fri, Feb 14, 2025 at 9:24 AM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On 2/13/25 12:30 PM, Olga Kornievskaia wrote: >>> On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote: >>>> >>>> On 2/13/25 10:47 AM, Olga Kornievskaia wrote: >>>>> Don't ignore return code of adding rdma listener. If nfs.conf has asked >>>>> for "rdma=y" but adding the listener fails, don't ignore the failure. >>>>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce), >>>>> when no address-constraints are used, an "any" listener is created and >>>>> rdma-enabling is done independently. >>>> >>>> This behavior is confusing... I suggest that an nfs.conf man page >>>> update accompany the below code change. >>> >>> Do you find only the rdma=y soft-rdma case confusing, or do you find >>> that when listeners fail and we shouldn't start knfsd threads in >>> general confusing? >>> >>> It was always the case that if rdma=y is done, then any listener >>> created for it does not check whether or not the underlying interface >>> is already rdma-enabled. This hasn't changed. Nor does this patch >>> change it. >> >> Not saying the patch changes the behavior. But you have to admit the >> behavior is surprising and needs clear documentation. > > Sure we can document the behavior of the any listener on a soft rdma > interface as it's used by the knfsd. But is it guaranteed not to > change, as the behaviour is controlled by the RDMA core not NFS? I'm talking about documenting the opposite. Something like: When "host=" is present, the network interface named by this configuration setting must be present (and when "rdma=y", that device must be RDMA-enabled) in order for server start-up to succeed. >>>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com> >>>> >>>> >>>>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") >>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> >>>>> --- >>>>> utils/nfsdctl/nfsdctl.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c >>>>> index 05fecc71..244910ef 100644 >>>>> --- a/utils/nfsdctl/nfsdctl.c >>>>> +++ b/utils/nfsdctl/nfsdctl.c >>>>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void) >>>>> if (tcp) >>>>> ret = add_listener("tcp", n->field, port); >>>>> if (rdma) >>>>> - add_listener("rdma", n->field, rdma_port); >>>>> + ret = add_listener("rdma", n->field, rdma_port); >>>>> if (ret) >>>>> return ret; >>>>> } >>>>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void) >>>>> if (tcp) >>>>> ret = add_listener("tcp", "", port); >>>>> if (rdma) >>>>> - add_listener("rdma", "", rdma_port); >>>>> + ret = add_listener("rdma", "", rdma_port); >>>>> } >>>>> return ret; >>>>> } >>>> >>>> >>>> -- >>>> Chuck Lever >>>> >>> >> >> >> -- >> Chuck Lever >>
On Fri, Feb 14, 2025 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 2/14/25 10:38 AM, Olga Kornievskaia wrote: > > On Fri, Feb 14, 2025 at 9:24 AM Chuck Lever <chuck.lever@oracle.com> wrote: > >> > >> On 2/13/25 12:30 PM, Olga Kornievskaia wrote: > >>> On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote: > >>>> > >>>> On 2/13/25 10:47 AM, Olga Kornievskaia wrote: > >>>>> Don't ignore return code of adding rdma listener. If nfs.conf has asked > >>>>> for "rdma=y" but adding the listener fails, don't ignore the failure. > >>>>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce), > >>>>> when no address-constraints are used, an "any" listener is created and > >>>>> rdma-enabling is done independently. > >>>> > >>>> This behavior is confusing... I suggest that an nfs.conf man page > >>>> update accompany the below code change. > >>> > >>> Do you find only the rdma=y soft-rdma case confusing, or do you find > >>> that when listeners fail and we shouldn't start knfsd threads in > >>> general confusing? > >>> > >>> It was always the case that if rdma=y is done, then any listener > >>> created for it does not check whether or not the underlying interface > >>> is already rdma-enabled. This hasn't changed. Nor does this patch > >>> change it. > >> > >> Not saying the patch changes the behavior. But you have to admit the > >> behavior is surprising and needs clear documentation. > > > > Sure we can document the behavior of the any listener on a soft rdma > > interface as it's used by the knfsd. But is it guaranteed not to > > change, as the behaviour is controlled by the RDMA core not NFS? > > I'm talking about documenting the opposite. Something like: > > When "host=" is present, the network interface named by this > configuration setting must be present (and when "rdma=y", that > device must be RDMA-enabled) in order for server start-up to > succeed. Ah that case. Yes, I agree. > >>>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > >>>> > >>>> > >>>>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") > >>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > >>>>> --- > >>>>> utils/nfsdctl/nfsdctl.c | 4 ++-- > >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > >>>>> index 05fecc71..244910ef 100644 > >>>>> --- a/utils/nfsdctl/nfsdctl.c > >>>>> +++ b/utils/nfsdctl/nfsdctl.c > >>>>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void) > >>>>> if (tcp) > >>>>> ret = add_listener("tcp", n->field, port); > >>>>> if (rdma) > >>>>> - add_listener("rdma", n->field, rdma_port); > >>>>> + ret = add_listener("rdma", n->field, rdma_port); > >>>>> if (ret) > >>>>> return ret; > >>>>> } > >>>>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void) > >>>>> if (tcp) > >>>>> ret = add_listener("tcp", "", port); > >>>>> if (rdma) > >>>>> - add_listener("rdma", "", rdma_port); > >>>>> + ret = add_listener("rdma", "", rdma_port); > >>>>> } > >>>>> return ret; > >>>>> } > >>>> > >>>> > >>>> -- > >>>> Chuck Lever > >>>> > >>> > >> > >> > >> -- > >> Chuck Lever > >> > > > -- > Chuck Lever
On Thu, 2025-02-13 at 10:47 -0500, Olga Kornievskaia wrote: > Don't ignore return code of adding rdma listener. If nfs.conf has asked > for "rdma=y" but adding the listener fails, don't ignore the failure. > Note in soft-rdma-provider environment (such as soft iwarp, soft roce), > when no address-constraints are used, an "any" listener is created and > rdma-enabling is done independently. > > Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> > --- > utils/nfsdctl/nfsdctl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c > index 05fecc71..244910ef 100644 > --- a/utils/nfsdctl/nfsdctl.c > +++ b/utils/nfsdctl/nfsdctl.c > @@ -1388,7 +1388,7 @@ static int configure_listeners(void) > if (tcp) > ret = add_listener("tcp", n->field, port); > if (rdma) > - add_listener("rdma", n->field, rdma_port); > + ret = add_listener("rdma", n->field, rdma_port); > if (ret) > return ret; > } > @@ -1398,7 +1398,7 @@ static int configure_listeners(void) > if (tcp) > ret = add_listener("tcp", "", port); > if (rdma) > - add_listener("rdma", "", rdma_port); > + ret = add_listener("rdma", "", rdma_port); > } > return ret; > } add_listener() is just adding the given listener to the nfsd_sockets array in memory. It's not consulting the kernel at that point. That won't happen until set_listeners() is called. It does make sense to check the return code though, just in case we pass it some bogus arguments somehow. So... Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c index 05fecc71..244910ef 100644 --- a/utils/nfsdctl/nfsdctl.c +++ b/utils/nfsdctl/nfsdctl.c @@ -1388,7 +1388,7 @@ static int configure_listeners(void) if (tcp) ret = add_listener("tcp", n->field, port); if (rdma) - add_listener("rdma", n->field, rdma_port); + ret = add_listener("rdma", n->field, rdma_port); if (ret) return ret; } @@ -1398,7 +1398,7 @@ static int configure_listeners(void) if (tcp) ret = add_listener("tcp", "", port); if (rdma) - add_listener("rdma", "", rdma_port); + ret = add_listener("rdma", "", rdma_port); } return ret; }
Don't ignore return code of adding rdma listener. If nfs.conf has asked for "rdma=y" but adding the listener fails, don't ignore the failure. Note in soft-rdma-provider environment (such as soft iwarp, soft roce), when no address-constraints are used, an "any" listener is created and rdma-enabling is done independently. Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed") Signed-off-by: Olga Kornievskaia <okorniev@redhat.com> --- utils/nfsdctl/nfsdctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)