[PATCH] generic: LVM and ram disks don't play well

[PATCH] generic: LVM and ram disks don't play well

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The "brd" kernel ram disk abuses BLKFLSBUF to mean "free all memory
in the ram drive" when in fact it actually means "flush all dirty
buffers to stable storage". the brd driver ignores BLKFLSBUF if
there is an active reference to the block device, (e.g. a fs is
mounted on it), but when a device is layered over the top of it
(e.g. dm-flakey, lvm devices, etc) then the applications and
filesystems hold references to the upper device, not the brd device.
Hence when the upper device passes down BLKFLSBUF to brd, it removes
all the pages in the brd, effectively erasing it.  This causes all
sorts of problems.....

Fix this by black listing "/dev/ramXXX" devices from tests that
require DM in some way. The _requires_sane_bdev_flush() macro is
called by the _requires_dm.... checks so that we don't have to
remember to add this to all new tests that use dm in some way.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 common/rc         | 34 +++++++++++++++++++++++-----------
 tests/generic/081 |  1 -
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/common/rc b/common/rc
index c5db0dd..ca8da7f 100644
--- a/common/rc
+++ b/common/rc
@@ -1305,26 +1305,38 @@ _require_block_device()
 	fi
 }
 
+# brd based ram disks erase the device when they receive a flush command when no
+# active references are present. This causes problems for DM devices sitting on
+# top of brd devices as DM doesn't hold active references to the brd device.
+_require_sane_bdev_flush()
+{
+	echo $1 | grep -q "^/dev/ram[0-9]\+$"
+	if [ $? -eq 0 ]; then
+		_notrun "This test requires a sane block device flush"
+	fi
+}
+
 # this test requires the device mapper flakey target
 #
 _require_dm_flakey()
 {
-    # require SCRATCH_DEV to be a valid block device
-    _require_block_device $SCRATCH_DEV
-    _require_command "$DMSETUP_PROG" dmsetup
+	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
+	# behaviour
+	_require_block_device $SCRATCH_DEV
+	_require_sane_bdev_flush $SCRATCH_DEV
+	_require_command "$DMSETUP_PROG" dmsetup
 
-    modprobe dm-flakey >/dev/null 2>&1
-    $DMSETUP_PROG targets | grep flakey >/dev/null 2>&1
-    if [ $? -eq 0 ]
-    then
-	:
-    else
-	_notrun "This test requires dm flakey support"
-    fi
+	modprobe dm-flakey >/dev/null 2>&1
+	$DMSETUP_PROG targets | grep flakey >/dev/null 2>&1
+	if [ $? -ne 0 ]; then
+		_notrun "This test requires dm flakey support"
+	fi
 }
 
 _require_dm_snapshot()
 {
+	_require_block_device $SCRATCH_DEV
+	_require_sane_bdev_flush $SCRATCH_DEV
 	_require_command "$DMSETUP_PROG" dmsetup
 	modprobe dm-snapshot >/dev/null 2>&1
 	$DMSETUP_PROG targets | grep -q snapshot
diff --git a/tests/generic/081 b/tests/generic/081
index e242c4c..5d38c11 100755
--- a/tests/generic/081
+++ b/tests/generic/081
@@ -50,7 +50,6 @@ _supported_os Linux
 _require_test
 _require_scratch_nocheck
 _require_dm_snapshot
-_require_block_device $SCRATCH_DEV
 _require_command $LVM_PROG lvm
 
 echo "Silence is golden"
-- 
2.0.0

Re: [PATCH] generic: LVM and ram disks don't play well

[Brian Foster]

On Tue, Apr 07, 2015 at 11:56:50AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The "brd" kernel ram disk abuses BLKFLSBUF to mean "free all memory
> in the ram drive" when in fact it actually means "flush all dirty
> buffers to stable storage". the brd driver ignores BLKFLSBUF if
> there is an active reference to the block device, (e.g. a fs is
> mounted on it), but when a device is layered over the top of it
> (e.g. dm-flakey, lvm devices, etc) then the applications and
> filesystems hold references to the upper device, not the brd device.
> Hence when the upper device passes down BLKFLSBUF to brd, it removes
> all the pages in the brd, effectively erasing it.  This causes all
> sorts of problems.....
> 
> Fix this by black listing "/dev/ramXXX" devices from tests that
> require DM in some way. The _requires_sane_bdev_flush() macro is
> called by the _requires_dm.... checks so that we don't have to
> remember to add this to all new tests that use dm in some way.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Sounds good to me... looks like we have a similar check in libxfs
(platform_flush_device()).

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  common/rc         | 34 +++++++++++++++++++++++-----------
>  tests/generic/081 |  1 -
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index c5db0dd..ca8da7f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1305,26 +1305,38 @@ _require_block_device()
>  	fi
>  }
>  
> +# brd based ram disks erase the device when they receive a flush command when no
> +# active references are present. This causes problems for DM devices sitting on
> +# top of brd devices as DM doesn't hold active references to the brd device.
> +_require_sane_bdev_flush()
> +{
> +	echo $1 | grep -q "^/dev/ram[0-9]\+$"
> +	if [ $? -eq 0 ]; then
> +		_notrun "This test requires a sane block device flush"
> +	fi
> +}
> +
>  # this test requires the device mapper flakey target
>  #
>  _require_dm_flakey()
>  {
> -    # require SCRATCH_DEV to be a valid block device
> -    _require_block_device $SCRATCH_DEV
> -    _require_command "$DMSETUP_PROG" dmsetup
> +	# require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF
> +	# behaviour
> +	_require_block_device $SCRATCH_DEV
> +	_require_sane_bdev_flush $SCRATCH_DEV
> +	_require_command "$DMSETUP_PROG" dmsetup
>  
> -    modprobe dm-flakey >/dev/null 2>&1
> -    $DMSETUP_PROG targets | grep flakey >/dev/null 2>&1
> -    if [ $? -eq 0 ]
> -    then
> -	:
> -    else
> -	_notrun "This test requires dm flakey support"
> -    fi
> +	modprobe dm-flakey >/dev/null 2>&1
> +	$DMSETUP_PROG targets | grep flakey >/dev/null 2>&1
> +	if [ $? -ne 0 ]; then
> +		_notrun "This test requires dm flakey support"
> +	fi
>  }
>  
>  _require_dm_snapshot()
>  {
> +	_require_block_device $SCRATCH_DEV
> +	_require_sane_bdev_flush $SCRATCH_DEV
>  	_require_command "$DMSETUP_PROG" dmsetup
>  	modprobe dm-snapshot >/dev/null 2>&1
>  	$DMSETUP_PROG targets | grep -q snapshot
> diff --git a/tests/generic/081 b/tests/generic/081
> index e242c4c..5d38c11 100755
> --- a/tests/generic/081
> +++ b/tests/generic/081
> @@ -50,7 +50,6 @@ _supported_os Linux
>  _require_test
>  _require_scratch_nocheck
>  _require_dm_snapshot
> -_require_block_device $SCRATCH_DEV
>  _require_command $LVM_PROG lvm
>  
>  echo "Silence is golden"
> -- 
> 2.0.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] generic: LVM and ram disks don't play well

[Theodore Ts'o]

On Tue, Apr 07, 2015 at 11:56:50AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The "brd" kernel ram disk abuses BLKFLSBUF to mean "free all memory
> in the ram drive" when in fact it actually means "flush all dirty
> buffers to stable storage".

Actually, what BLKFLSBUF means is to "flush all dirty buffers to
stable storage --- and then invalidate/kill all buffers for the
underyling block device."

The reason why the ramdisk driver has special semantics BLKFLSBUF is
an accident of how it was originally implemented, back in the 0.1x
days of Linux, where it was implemented as a set of buffers in the
buffer cache that weren't actually backed by any device.  Since they
were marked dirty, they wouldn't actually get dropped by the buffer
cache.  It was a dirty hack, but back in the days where they were used
to populate the ramdisk from a floppy disk, it worked well enough.

It happened that how BLKFLSBUF was implemented, without having to do
anything special, the fact that BLKFLSBUF would invalidate all of the
buffers from the buffer cache, userspace code would use this as a way
of causing the ram disk to release memory.

Given the desire not to break userspace, when the brd device was
implemented to no longer depend on the buffer cache, the "special"
semantics of BLKFLSBUF were preserved.  We probably should have
allocated a new ioctl for the explicit purpose of releasing memory,
and then tracked down all users of the magic BLKFLSBUF ioctl.  I don't
think there should be that many --- in fact, it may very well be that
init/do_mounts_initrd.c in the kernel sources might be one of the few
still around, since the days of boot/root floppies are long before us.

Cheers,

						- Ted