[PATCH 2/3] nbd: fsync and kill block device on shutdown

[PATCH 2/3] nbd: fsync and kill block device on shutdown

[Paolo Bonzini]

There are two problems with shutdown in the NBD driver.  The first is
that receiving the NBD_DISCONNECT ioctl does not sync the filesystem;
this is useful because BLKFLSBUF is restricted to processes that have
CAP_SYS_ADMIN, and the NBD client may not possess it (fsync of the
block device does not sync the filesystem, either).

The second is that once we clear the socket we have no guarantee that
later reads will come from the same backing storage.  Thus the page cache
must be cleaned, lest reads that hit on the page cache will return stale
data from the previously-accessible disk.

Example:

    # qemu-nbd -r -c/dev/nbd0 /dev/sr0
    # file -s /dev/nbd0
    /dev/stdin: # UDF filesystem data (version 1.5) etc.
    # qemu-nbd -d /dev/nbd0
    # qemu-nbd -r -c/dev/nbd0 /dev/sda
    # file -s /dev/nbd0
    /dev/stdin: # UDF filesystem data (version 1.5) etc.

While /dev/sda has:

    # file -s /dev/sda
    /dev/sda: x86 boot sector; etc.

Cc: <stable@vger.kernel.org>
Cc: <nbd-general@lists.sf.net>
Cc: Paul Clements <Paul.Clements@steeleye.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/block/nbd.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5603765..a9c5c7a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -608,12 +608,20 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		struct request sreq;
 
 		dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
+		if (!nbd->sock)
+			return -EINVAL;
 
+		mutex_unlock(&nbd->tx_lock);
+		fsync_bdev(bdev);
+		mutex_lock(&nbd->tx_lock);
 		blk_rq_init(NULL, &sreq);
 		sreq.cmd_type = REQ_TYPE_SPECIAL;
 		nbd_cmd(&sreq) = NBD_CMD_DISC;
+
+		/* Check again after getting mutex back.  */
 		if (!nbd->sock)
 			return -EINVAL;
+
 		nbd_send_req(nbd, &sreq);
                 return 0;
 	}
@@ -627,6 +635,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		nbd_clear_que(nbd);
 		BUG_ON(!list_empty(&nbd->queue_head));
 		BUG_ON(!list_empty(&nbd->waiting_queue));
+		kill_bdev(bdev);
 		if (file)
 			fput(file);
 		return 0;
@@ -719,6 +728,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		nbd->file = NULL;
 		nbd_clear_que(nbd);
 		dev_warn(disk_to_dev(nbd->disk), "queue cleared\n");
+		kill_bdev(bdev);
 		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
 		if (file)
 			fput(file);
-- 
1.7.1

Re: [PATCH 2/3] nbd: fsync and kill block device on shutdown

[Andrew Morton]

On Tue, 12 Feb 2013 17:06:10 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> There are two problems with shutdown in the NBD driver.  The first is
> that receiving the NBD_DISCONNECT ioctl does not sync the filesystem;
> this is useful because BLKFLSBUF is restricted to processes that have
> CAP_SYS_ADMIN, and the NBD client may not possess it (fsync of the
> block device does not sync the filesystem, either).

hm, this says that the lack of a sync is "useful".  I think you mean
that the patch-which-adds-the-sync is the thing which is useful, yes?

> The second is that once we clear the socket we have no guarantee that
> later reads will come from the same backing storage.  Thus the page cache
> must be cleaned, lest reads that hit on the page cache will return stale
> data from the previously-accessible disk.

That sounds like a problem.

> Example:
> 
>     # qemu-nbd -r -c/dev/nbd0 /dev/sr0
>     # file -s /dev/nbd0
>     /dev/stdin: # UDF filesystem data (version 1.5) etc.
>     # qemu-nbd -d /dev/nbd0
>     # qemu-nbd -r -c/dev/nbd0 /dev/sda
>     # file -s /dev/nbd0
>     /dev/stdin: # UDF filesystem data (version 1.5) etc.
> 
> While /dev/sda has:
> 
>     # file -s /dev/sda
>     /dev/sda: x86 boot sector; etc.

OK, we've described the problems but there's no description here of how
the patch addresses those problems.


How does this look?


: There are two problems with shutdown in the NBD driver.
: 
: 1: Receiving the NBD_DISCONNECT ioctl does not sync the filesystem. 
: 
:    This patch adds the sync operation into __nbd_ioctl()'s
:    NBD_DISCONNECT handler.  This is useful because BLKFLSBUF is restricted
:    to processes that have CAP_SYS_ADMIN, and the NBD client may not
:    possess it (fsync of the block device does not sync the filesystem,
:    either).
: 
: 2: Once we clear the socket we have no guarantee that later reads will
:    come from the same backing storage.
: 
:    The patch adds calls to kill_bdev() in __nbd_ioctl()'s socket
:    clearing code so the page cache is cleaned, lest reads that hit on the
:    page cache will return stale data from the previously-accessible disk.
: 
: Example:
: 
:     # qemu-nbd -r -c/dev/nbd0 /dev/sr0
:     # file -s /dev/nbd0
:     /dev/stdin: # UDF filesystem data (version 1.5) etc.
:     # qemu-nbd -d /dev/nbd0
:     # qemu-nbd -r -c/dev/nbd0 /dev/sda
:     # file -s /dev/nbd0
:     /dev/stdin: # UDF filesystem data (version 1.5) etc.
: 
: While /dev/sda has:
: 
:     # file -s /dev/sda
:     /dev/sda: x86 boot sector; etc.

Re: [PATCH 2/3] nbd: fsync and kill block device on shutdown

[Paul Clements]

On Tue, Feb 12, 2013 at 11:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> There are two problems with shutdown in the NBD driver.  The first is
> that receiving the NBD_DISCONNECT ioctl does not sync the filesystem;
> this is useful because BLKFLSBUF is restricted to processes that have
> CAP_SYS_ADMIN, and the NBD client may not possess it (fsync of the
> block device does not sync the filesystem, either).

> The second is that once we clear the socket we have no guarantee that
> later reads will come from the same backing storage.  Thus the page cache
> must be cleaned, lest reads that hit on the page cache will return stale
> data from the previously-accessible disk.

Paolo,

Thanks for this. A problem indeed...

Acked-by: Paul.Clements@steeleye.com


> Cc: <stable@vger.kernel.org>
> Cc: <nbd-general@lists.sf.net>
> Cc: Paul Clements <Paul.Clements@steeleye.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  drivers/block/nbd.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 5603765..a9c5c7a 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -608,12 +608,20 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 struct request sreq;
>
>                 dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
> +               if (!nbd->sock)
> +                       return -EINVAL;
>
> +               mutex_unlock(&nbd->tx_lock);
> +               fsync_bdev(bdev);
> +               mutex_lock(&nbd->tx_lock);
>                 blk_rq_init(NULL, &sreq);
>                 sreq.cmd_type = REQ_TYPE_SPECIAL;
>                 nbd_cmd(&sreq) = NBD_CMD_DISC;
> +
> +               /* Check again after getting mutex back.  */
>                 if (!nbd->sock)
>                         return -EINVAL;
> +
>                 nbd_send_req(nbd, &sreq);
>                  return 0;
>         }
> @@ -627,6 +635,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 nbd_clear_que(nbd);
>                 BUG_ON(!list_empty(&nbd->queue_head));
>                 BUG_ON(!list_empty(&nbd->waiting_queue));
> +               kill_bdev(bdev);
>                 if (file)
>                         fput(file);
>                 return 0;
> @@ -719,6 +728,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 nbd->file = NULL;
>                 nbd_clear_que(nbd);
>                 dev_warn(disk_to_dev(nbd->disk), "queue cleared\n");
> +               kill_bdev(bdev);
>                 queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>                 if (file)
>                         fput(file);
> --
> 1.7.1
>
>

Re: [PATCH 2/3] nbd: fsync and kill block device on shutdown

[Paolo Bonzini]

Il 12/02/2013 22:41, Andrew Morton ha scritto:
>> > There are two problems with shutdown in the NBD driver.  The first is
>> > that receiving the NBD_DISCONNECT ioctl does not sync the filesystem;
>> > this is useful because BLKFLSBUF is restricted to processes that have
>> > CAP_SYS_ADMIN, and the NBD client may not possess it (fsync of the
>> > block device does not sync the filesystem, either).
> hm, this says that the lack of a sync is "useful".  I think you mean
> that the patch-which-adds-the-sync is the thing which is useful, yes?

Yes.

>> > The second is that once we clear the socket we have no guarantee that
>> > later reads will come from the same backing storage.  Thus the page cache
>> > must be cleaned, lest reads that hit on the page cache will return stale
>> > data from the previously-accessible disk.
> That sounds like a problem.
> 
>> > Example:
>> > 
>> >     # qemu-nbd -r -c/dev/nbd0 /dev/sr0
>> >     # file -s /dev/nbd0
>> >     /dev/stdin: # UDF filesystem data (version 1.5) etc.
>> >     # qemu-nbd -d /dev/nbd0
>> >     # qemu-nbd -r -c/dev/nbd0 /dev/sda
>> >     # file -s /dev/nbd0
>> >     /dev/stdin: # UDF filesystem data (version 1.5) etc.
>> > 
>> > While /dev/sda has:
>> > 
>> >     # file -s /dev/sda
>> >     /dev/sda: x86 boot sector; etc.
> OK, we've described the problems but there's no description here of how
> the patch addresses those problems.
> 
> How does this look?

Perfect, thanks very much.  I tried to similarly balance the "why" and
"how" in the new commit message for patch 1.

Paolo