* Re: [PATCH 2/3] nbd: fsync and kill block device on shutdown
2013-02-12 16:06 ` [PATCH 2/3] nbd: fsync and kill block device on shutdown Paolo Bonzini
@ 2013-02-12 21:41 ` Andrew Morton
2013-02-13 13:05 ` Paolo Bonzini
2013-02-12 22:15 ` Paul Clements
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2013-02-12 21:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, stable, nbd-general, Paul Clements
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] nbd: fsync and kill block device on shutdown
2013-02-12 16:06 ` [PATCH 2/3] nbd: fsync and kill block device on shutdown Paolo Bonzini
2013-02-12 21:41 ` Andrew Morton
@ 2013-02-12 22:15 ` Paul Clements
1 sibling, 0 replies; 4+ messages in thread
From: Paul Clements @ 2013-02-12 22:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, stable, nbd-general, Andrew Morton
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread