qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH V2 0/2] add support for nfs_umount
@ 2019-09-10 15:41 Peter Lieven
  2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close Peter Lieven
  2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount Peter Lieven
  0 siblings, 2 replies; 10+ messages in thread
From: Peter Lieven @ 2019-09-10 15:41 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, Peter Lieven, qemu-devel, ronniesahlberg, mreitz

add support for NFSv3 umount call. V2 adds a patch that fixes
the order of the aio teardown. The addition of the NFS umount
call unmasked that bug.

Peter Lieven (2):
  block/nfs: tear down aio before nfs_close
  block/nfs: add support for nfs_umount

 block/nfs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.17.1




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

* [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close
  2019-09-10 15:41 [Qemu-devel] [PATCH V2 0/2] add support for nfs_umount Peter Lieven
@ 2019-09-10 15:41 ` Peter Lieven
  2019-09-13  9:51   ` Max Reitz
  2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount Peter Lieven
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Lieven @ 2019-09-10 15:41 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, Peter Lieven, qemu-stable, qemu-devel, ronniesahlberg, mreitz

nfs_close is a sync call from libnfs and has its own event
handler polling on the nfs FD. Avoid that both QEMU and libnfs
are intefering here.

CC: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/nfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 0ec50953e4..2c98508275 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -390,12 +390,14 @@ static void nfs_attach_aio_context(BlockDriverState *bs,
 static void nfs_client_close(NFSClient *client)
 {
     if (client->context) {
+        qemu_mutex_lock(&client->mutex);
+        aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
+                           false, NULL, NULL, NULL, NULL);
+        qemu_mutex_unlock(&client->mutex);
         if (client->fh) {
             nfs_close(client->context, client->fh);
             client->fh = NULL;
         }
-        aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
-                           false, NULL, NULL, NULL, NULL);
         nfs_destroy_context(client->context);
         client->context = NULL;
     }
-- 
2.17.1




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

* [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount
  2019-09-10 15:41 [Qemu-devel] [PATCH V2 0/2] add support for nfs_umount Peter Lieven
  2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close Peter Lieven
@ 2019-09-10 15:41 ` Peter Lieven
  2019-09-11  7:48   ` Max Reitz
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Lieven @ 2019-09-10 15:41 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, Peter Lieven, qemu-devel, ronniesahlberg, mreitz

libnfs recently added support for unmounting. Add support
in Qemu too.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/nfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/nfs.c b/block/nfs.c
index 2c98508275..f39acfdb28 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -398,6 +398,9 @@ static void nfs_client_close(NFSClient *client)
             nfs_close(client->context, client->fh);
             client->fh = NULL;
         }
+#ifdef LIBNFS_FEATURE_UMOUNT
+        nfs_umount(client->context);
+#endif
         nfs_destroy_context(client->context);
         client->context = NULL;
     }
-- 
2.17.1




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

* Re: [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount
  2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount Peter Lieven
@ 2019-09-11  7:48   ` Max Reitz
  2019-09-11 12:22     ` Peter Lieven
  2019-09-13 10:09     ` ronnie sahlberg
  0 siblings, 2 replies; 10+ messages in thread
From: Max Reitz @ 2019-09-11  7:48 UTC (permalink / raw)
  To: Peter Lieven, qemu-block; +Cc: kwolf, qemu-devel, ronniesahlberg


[-- Attachment #1.1: Type: text/plain, Size: 921 bytes --]

On 10.09.19 17:41, Peter Lieven wrote:
> libnfs recently added support for unmounting. Add support
> in Qemu too.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/nfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index 2c98508275..f39acfdb28 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -398,6 +398,9 @@ static void nfs_client_close(NFSClient *client)
>              nfs_close(client->context, client->fh);
>              client->fh = NULL;
>          }
> +#ifdef LIBNFS_FEATURE_UMOUNT
> +        nfs_umount(client->context);
> +#endif
>          nfs_destroy_context(client->context);
>          client->context = NULL;
>      }

I don’t understand what unmounting means in this context.  Is it just
generic clean-up for NFSv3 (it appears that it’s a no-op for NFSv4)?
Why isn’t that done by nfs_destroy_context()?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount
  2019-09-11  7:48   ` Max Reitz
@ 2019-09-11 12:22     ` Peter Lieven
  2019-09-13 10:09     ` ronnie sahlberg
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Lieven @ 2019-09-11 12:22 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: kwolf, qemu-devel, ronniesahlberg

Am 11.09.19 um 09:48 schrieb Max Reitz:
> On 10.09.19 17:41, Peter Lieven wrote:
>> libnfs recently added support for unmounting. Add support
>> in Qemu too.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/nfs.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/block/nfs.c b/block/nfs.c
>> index 2c98508275..f39acfdb28 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -398,6 +398,9 @@ static void nfs_client_close(NFSClient *client)
>>              nfs_close(client->context, client->fh);
>>              client->fh = NULL;
>>          }
>> +#ifdef LIBNFS_FEATURE_UMOUNT
>> +        nfs_umount(client->context);
>> +#endif
>>          nfs_destroy_context(client->context);
>>          client->context = NULL;
>>      }
> I don’t understand what unmounting means in this context.  Is it just
> generic clean-up for NFSv3 (it appears that it’s a no-op for NFSv4)?


Its a call to the mount daemon on the NFSv3 server. It will effectively

cause that the connection is no longer listed when showmounts -a is issued on the server.


> Why isn’t that done by nfs_destroy_context()?


That would have been the right place, but libnfs added support for this call only recently. I think with version 4.0.0


Peter





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

* Re: [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close
  2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close Peter Lieven
@ 2019-09-13  9:51   ` Max Reitz
  2019-09-13 10:15     ` Peter Lieven
  2019-09-13 10:21     ` Kevin Wolf
  0 siblings, 2 replies; 10+ messages in thread
From: Max Reitz @ 2019-09-13  9:51 UTC (permalink / raw)
  To: Peter Lieven, qemu-block; +Cc: kwolf, qemu-devel, ronniesahlberg, qemu-stable


[-- Attachment #1.1: Type: text/plain, Size: 726 bytes --]

On 10.09.19 17:41, Peter Lieven wrote:
> nfs_close is a sync call from libnfs and has its own event
> handler polling on the nfs FD. Avoid that both QEMU and libnfs
> are intefering here.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/nfs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

I’ve just seen that Kevin has already included the second patch (in its
v1) in his pull request.

So all that I can do is take this patch, which sounds good to me,
especially since Ronnie has agreed that we should remove our FD handler
there.

(So I’ve rebased the patch on top of Kevin’s pull request, and I’ve
taken it to my block branch.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount
  2019-09-11  7:48   ` Max Reitz
  2019-09-11 12:22     ` Peter Lieven
@ 2019-09-13 10:09     ` ronnie sahlberg
  2019-09-13 11:13       ` Max Reitz
  1 sibling, 1 reply; 10+ messages in thread
From: ronnie sahlberg @ 2019-09-13 10:09 UTC (permalink / raw)
  To: Max Reitz
  Cc: Kevin Wolf, Peter Lieven, qemu-devel, open list:Block layer core

On Wed, Sep 11, 2019 at 5:48 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 10.09.19 17:41, Peter Lieven wrote:
> > libnfs recently added support for unmounting. Add support
> > in Qemu too.
> >
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> >  block/nfs.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/block/nfs.c b/block/nfs.c
> > index 2c98508275..f39acfdb28 100644
> > --- a/block/nfs.c
> > +++ b/block/nfs.c
> > @@ -398,6 +398,9 @@ static void nfs_client_close(NFSClient *client)
> >              nfs_close(client->context, client->fh);
> >              client->fh = NULL;
> >          }
> > +#ifdef LIBNFS_FEATURE_UMOUNT
> > +        nfs_umount(client->context);
> > +#endif
> >          nfs_destroy_context(client->context);
> >          client->context = NULL;
> >      }
>
> I don’t understand what unmounting means in this context.  Is it just
> generic clean-up for NFSv3 (it appears that it’s a no-op for NFSv4)?
> Why isn’t that done by nfs_destroy_context()?

Umount is weird since there isn't actually any state in NFSv3 and
"mounting" in nfsv3 is really just a matter of converting the path to
be mounted into a filehandle.
That is all the mount protocol is really used for.

This is all handled in a separate protocol/server called rpc.mountd
that is separate from NFSd. Running as a different process and
listening to a different port.
And the only purpose of rpc.mountd is to take a path to a share and
return a nfsv3 filehandle to the root of that path.
As a side-effect, rpc.mountd also keeps track of which clients have
called MNT but not yet UMNT and thus showmount -a
can give a lost of all client that have "mounted" the share but not
yet called "UMNT".

It has no effect at all on NFSv3 and is purely cosmetic. This ONLY
affects "showmount -a" output.
NFSv4 does away with all these separate protocols such as mount,
statd, nlm and even portmapper
so there is not even a concept of showmount -a for nfsv4.


As the libnfs maintainer, why did I do nfs_umount() the way I did?
First of all, I think of nfs UMNT as really just cosmetic and didn't
want to put too much work into it. But people wanted it.

I implemented it as a sync function since I think few people would
actually use it at all and it meant that I just didn't have to invest
in having to build an async piupelinje.

I did NOT implement it inside nfs_destroy_context() since that
function is supposed to be, in my view, non-blocking andn should just
tear the connection and immediately return.
As unmount would be
* close the tcp socket to the nfs server
* open new socket to portmapper and ask "where is rpc.mountd"
* close socket to portmapper, then open new socket to rpc.mountd
 * tell rpc.mountd to remove us from the showmount -a list
* close socket

I just took the cheap and easy path. Do it as a sync function with my
own eventloop.

Again, UMNT has no real effect on anything related to NFS except what
showmount -a will return. That is one big reason why
I was just not much motivated enough to build it as an async function.

Once we all switch to NFSv4 this will all be moot since the MOUNT
protocol no longer exists and neither does rpc.mountd.



>
> Max
>


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

* Re: [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close
  2019-09-13  9:51   ` Max Reitz
@ 2019-09-13 10:15     ` Peter Lieven
  2019-09-13 10:21     ` Kevin Wolf
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Lieven @ 2019-09-13 10:15 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: kwolf, qemu-devel, ronniesahlberg, qemu-stable

Am 13.09.19 um 11:51 schrieb Max Reitz:
> On 10.09.19 17:41, Peter Lieven wrote:
>> nfs_close is a sync call from libnfs and has its own event
>> handler polling on the nfs FD. Avoid that both QEMU and libnfs
>> are intefering here.
>>
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/nfs.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
> I’ve just seen that Kevin has already included the second patch (in its
> v1) in his pull request.
>
> So all that I can do is take this patch, which sounds good to me,
> especially since Ronnie has agreed that we should remove our FD handler
> there.
>
> (So I’ve rebased the patch on top of Kevin’s pull request, and I’ve
> taken it to my block branch.)


Thank you. After I discovered that we had this bug also before I added the nfs_umount call I thought

it would be good to have this patch first and the add the umount call because the fix should also go into

stable because in theory we could also run into trouble with just the *sync* nfs_clsoe call.


Peter




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

* Re: [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close
  2019-09-13  9:51   ` Max Reitz
  2019-09-13 10:15     ` Peter Lieven
@ 2019-09-13 10:21     ` Kevin Wolf
  1 sibling, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2019-09-13 10:21 UTC (permalink / raw)
  To: Max Reitz
  Cc: ronniesahlberg, Peter Lieven, qemu-devel, qemu-block, qemu-stable

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

Am 13.09.2019 um 11:51 hat Max Reitz geschrieben:
> On 10.09.19 17:41, Peter Lieven wrote:
> > nfs_close is a sync call from libnfs and has its own event
> > handler polling on the nfs FD. Avoid that both QEMU and libnfs
> > are intefering here.
> > 
> > CC: qemu-stable@nongnu.org
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> >  block/nfs.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> I’ve just seen that Kevin has already included the second patch (in its
> v1) in his pull request.

Oops, sorry about this. Peter hasn't pulled yet, so I'll update the tag
for the pull request. Let's see which version gets merged in the end.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

* Re: [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount
  2019-09-13 10:09     ` ronnie sahlberg
@ 2019-09-13 11:13       ` Max Reitz
  0 siblings, 0 replies; 10+ messages in thread
From: Max Reitz @ 2019-09-13 11:13 UTC (permalink / raw)
  To: ronnie sahlberg
  Cc: Kevin Wolf, Peter Lieven, qemu-devel, open list:Block layer core


[-- Attachment #1.1: Type: text/plain, Size: 3476 bytes --]

On 13.09.19 12:09, ronnie sahlberg wrote:
> On Wed, Sep 11, 2019 at 5:48 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 10.09.19 17:41, Peter Lieven wrote:
>>> libnfs recently added support for unmounting. Add support
>>> in Qemu too.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>  block/nfs.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/block/nfs.c b/block/nfs.c
>>> index 2c98508275..f39acfdb28 100644
>>> --- a/block/nfs.c
>>> +++ b/block/nfs.c
>>> @@ -398,6 +398,9 @@ static void nfs_client_close(NFSClient *client)
>>>              nfs_close(client->context, client->fh);
>>>              client->fh = NULL;
>>>          }
>>> +#ifdef LIBNFS_FEATURE_UMOUNT
>>> +        nfs_umount(client->context);
>>> +#endif
>>>          nfs_destroy_context(client->context);
>>>          client->context = NULL;
>>>      }
>>
>> I don’t understand what unmounting means in this context.  Is it just
>> generic clean-up for NFSv3 (it appears that it’s a no-op for NFSv4)?
>> Why isn’t that done by nfs_destroy_context()?
> 
> Umount is weird since there isn't actually any state in NFSv3 and
> "mounting" in nfsv3 is really just a matter of converting the path to
> be mounted into a filehandle.
> That is all the mount protocol is really used for.
> 
> This is all handled in a separate protocol/server called rpc.mountd
> that is separate from NFSd. Running as a different process and
> listening to a different port.
> And the only purpose of rpc.mountd is to take a path to a share and
> return a nfsv3 filehandle to the root of that path.
> As a side-effect, rpc.mountd also keeps track of which clients have
> called MNT but not yet UMNT and thus showmount -a
> can give a lost of all client that have "mounted" the share but not
> yet called "UMNT".
> 
> It has no effect at all on NFSv3 and is purely cosmetic. This ONLY
> affects "showmount -a" output.
> NFSv4 does away with all these separate protocols such as mount,
> statd, nlm and even portmapper
> so there is not even a concept of showmount -a for nfsv4.
> 
> 
> As the libnfs maintainer, why did I do nfs_umount() the way I did?
> First of all, I think of nfs UMNT as really just cosmetic and didn't
> want to put too much work into it. But people wanted it.
> 
> I implemented it as a sync function since I think few people would
> actually use it at all and it meant that I just didn't have to invest
> in having to build an async piupelinje.
> 
> I did NOT implement it inside nfs_destroy_context() since that
> function is supposed to be, in my view, non-blocking andn should just
> tear the connection and immediately return.
> As unmount would be
> * close the tcp socket to the nfs server
> * open new socket to portmapper and ask "where is rpc.mountd"
> * close socket to portmapper, then open new socket to rpc.mountd
>  * tell rpc.mountd to remove us from the showmount -a list
> * close socket
> 
> I just took the cheap and easy path. Do it as a sync function with my
> own eventloop.
> 
> Again, UMNT has no real effect on anything related to NFS except what
> showmount -a will return. That is one big reason why
> I was just not much motivated enough to build it as an async function.
> 
> Once we all switch to NFSv4 this will all be moot since the MOUNT
> protocol no longer exists and neither does rpc.mountd.

OK.  Thanks a lot for the detailed explanation! :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

end of thread, other threads:[~2019-09-13 11:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-10 15:41 [Qemu-devel] [PATCH V2 0/2] add support for nfs_umount Peter Lieven
2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 1/2] block/nfs: tear down aio before nfs_close Peter Lieven
2019-09-13  9:51   ` Max Reitz
2019-09-13 10:15     ` Peter Lieven
2019-09-13 10:21     ` Kevin Wolf
2019-09-10 15:41 ` [Qemu-devel] [PATCH V2 2/2] block/nfs: add support for nfs_umount Peter Lieven
2019-09-11  7:48   ` Max Reitz
2019-09-11 12:22     ` Peter Lieven
2019-09-13 10:09     ` ronnie sahlberg
2019-09-13 11:13       ` Max Reitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).