Re: [Qemu-devel] [PATCH] block/nfs: add support for nfs_umount

[Kevin Wolf <kwolf@redhat.com>]

Am 03.09.2019 um 21:52 hat Peter Lieven geschrieben:
> 
> 
> > Am 03.09.2019 um 16:56 schrieb Kevin Wolf <kwolf@redhat.com>:
> > 
> > Am 03.09.2019 um 15:44 hat Peter Lieven geschrieben:
> >> libnfs recently added support for unmounting. Add support
> >> in Qemu too.
> >> 
> >> Signed-off-by: Peter Lieven <pl@kamp.de>
> > 
> > Looks trivial enough to review even for me. :-)
> > 
> > Thanks, applied to the block branch.
> > 
> > Kevin
> 
> I am not sure what the reason is, but with this patch I sometimes run
> into nfs_process_read being called for a cdrom mounted from nfs after
> I ejected it (and the whole nfs client context is already destroyed).

Does this mean that nfs_umount() gets some response, but we don't
properly wait for it? Or is some older request still in flight?

> It seems that the following fixes the issue. It might be that the
> nfs_umount call just reveals this bug. nfs_close is also doing sync
> I/O with the libnfs library. If we mount a nfs share we are doing
> everything with sync calls and then set up the aio stuff with
> nfs_set_events. I think that we need to stop the aio before we are
> executing the sync calls to nfs_close and nfs_umount.

Disabling the fd handlers is probably just papering over the problem. If
there is something in flight, we should wait for its completion. In
theory, block devices are drained before calling .bdrv_close, so
either we have a bug with draining nfs nodes correctly, or .bdrv_close
initiates something new itself that we should wait for.

> Also not sure if holding the mutex is necessary here.

Not for aio_set_fd_handler(), but I don't know the requirements of
nfs_get_fd().

Kevin

> diff --git a/block/nfs.c b/block/nfs.c
> index ffa6484c1a..cb2e0d375a 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -389,6 +389,10 @@ 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;
> @@ -396,8 +400,6 @@ static void nfs_client_close(NFSClient *client)
>  #ifdef LIBNFS_FEATURE_UMOUNT
>          nfs_umount(client->context);
>  #endif
> -        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;
>      }