[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Li Wang]

We get failures like below because of 'DEVICE_SIZE-10000' makes the
loop device size is not aligned to system PAGE_SIZE any more.

-----
mkswap01 1 TINFO : Found free device '/dev/loop1'
mkswap01 1 TPASS : 'mkswap   /dev/loop1 ' passed.
mkswap01 2 TFAIL : 'mkswap -f  /dev/loop1 92400' failed, not expected.
mkswap01 3 TINFO : Can not do swapon on /dev/loop1.
mkswap01 3 TINFO : Device size specified by 'mkswap' greater than real size.
mkswap01 3 TINFO : Swapon failed expectedly.
mkswap01 3 TPASS : 'mkswap -f  /dev/loop1 112400' passed.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/commands/mkswap/mkswap01.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/testcases/commands/mkswap/mkswap01.sh b/testcases/commands/mkswap/mkswap01.sh
index 9727b0f..e89c720 100755
--- a/testcases/commands/mkswap/mkswap01.sh
+++ b/testcases/commands/mkswap/mkswap01.sh
@@ -169,8 +169,8 @@ mkswap_test()
 setup
 
 mkswap_test "" "" "$TST_DEVICE"
-mkswap_test "" "" "$TST_DEVICE" "$((DEVICE_SIZE-10000))"
-mkswap_test "-f" "" "$TST_DEVICE" "$((DEVICE_SIZE+10000))"
+mkswap_test "" "" "$TST_DEVICE" "$((DEVICE_SIZE-PAGE_SIZE/1024))"
+mkswap_test "-f" "" "$TST_DEVICE" "$((DEVICE_SIZE+PAGE_SIZE/1024))"
 mkswap_test "-c" "" "$TST_DEVICE"
 mkswap_test "-p" "2048" "$TST_DEVICE"
 mkswap_test "-L" "ltp_testswap" "-L ltp_testswap" "" "/dev/disk/by-label/ltp_testswap"
-- 
1.8.3.1

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Cyril Hrubis]

Hi!
> We get failures like below because of 'DEVICE_SIZE-10000' makes the
> loop device size is not aligned to system PAGE_SIZE any more.

What version of mkswap.c you have?

Looking into util-linux-2.26.2 all it does with user specified number of
blocks is:

ctl.npages = blks / (ctl.pagesize / 1024);

There is no check that blks is divideable by pagesize/1024.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Li Wang]

On Tue, Aug 09, 2016 at 06:41:18PM +0200, Cyril Hrubis wrote:
> Hi!
> > We get failures like below because of 'DEVICE_SIZE-10000' makes the
> > loop device size is not aligned to system PAGE_SIZE any more.
> 
> What version of mkswap.c you have?

My version is:

# rpm -qf /sbin/mkswap 
util-linux-2.23.2-33.el7.ppc64


if (block_count) {
	/* this silly user specified the number of blocks explicitly */
		uint64_t blks = strtou64_or_err(block_count,
					_("invalid block count argument"));
		PAGES = blks / (pagesize / 1024);
	}
	sz = get_size(device_name);


> 
> Looking into util-linux-2.26.2 all it does with user specified number of
> blocks is:
> 
> ctl.npages = blks / (ctl.pagesize / 1024);
> 
> There is no check that blks is divideable by pagesize/1024.

I build util-linux-2.28 from upstrem source and also get the errors.

# mkswap -V
mkswap from util-linux 2.28.237-47bd89

# ./runltp -f commands
mkswap01 1 TINFO : Using test device LTP_DEV='/dev/loop0'
mkswap01 1 TFAIL : 'mkswap   /dev/loop0 ' failed, not expected.
mkswap01 2 TFAIL : 'mkswap   /dev/loop0 90000' failed, not expected.
mkswap01 3 TINFO : Can not do swapon on /dev/loop0.
mkswap01 3 TINFO : Device size specified by 'mkswap' greater than real size.
mkswap01 3 TINFO : Swapon failed expectedly.
mkswap01 3 TPASS : 'mkswap -f  /dev/loop0 110000' passed.

-----------------

Additionally, we'd better change the bs=1kB to bs=1024 in runltp file,
since it will get page aligned block device for all arches.

The original LTP_DEV size is 102400000 bytes:

# grep 'bs=' runltp  -n
981:    dd if=/dev/zero of=${TMP}/test.img bs=1kB count=102400


From dd manual:
N and BYTES may be followed by the following multiplicative suffixes:
c =1, w =2, b =512, kB  =1000,  K  =1024,  MB =1000*1000,  M  =1024*1024,
xM  =M  GB  =1000*1000*1000,  G =1024*1024*1024, and so on for T, P, E, Z, Y.


Regards,
Li Wang

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Cyril Hrubis]

Hi!
> My version is:
> 
> # rpm -qf /sbin/mkswap 
> util-linux-2.23.2-33.el7.ppc64
> 
> 
> if (block_count) {
> 	/* this silly user specified the number of blocks explicitly */
> 		uint64_t blks = strtou64_or_err(block_count,
> 					_("invalid block count argument"));
> 		PAGES = blks / (pagesize / 1024);
> 	}
> 	sz = get_size(device_name);

There is no check here as well. The strtou64_or_err() just converts
string to number, that's all it does. Then we do integer division, no
checks at all.

> > Looking into util-linux-2.26.2 all it does with user specified number of
> > blocks is:
> > 
> > ctl.npages = blks / (ctl.pagesize / 1024);
> > 
> > There is no check that blks is divideable by pagesize/1024.
> 
> I build util-linux-2.28 from upstrem source and also get the errors.
> 
> # mkswap -V
> mkswap from util-linux 2.28.237-47bd89
> 
> # ./runltp -f commands
> mkswap01 1 TINFO : Using test device LTP_DEV='/dev/loop0'
> mkswap01 1 TFAIL : 'mkswap   /dev/loop0 ' failed, not expected.
> mkswap01 2 TFAIL : 'mkswap   /dev/loop0 90000' failed, not expected.
> mkswap01 3 TINFO : Can not do swapon on /dev/loop0.
> mkswap01 3 TINFO : Device size specified by 'mkswap' greater than real size.
> mkswap01 3 TINFO : Swapon failed expectedly.
> mkswap01 3 TPASS : 'mkswap -f  /dev/loop0 110000' passed.

Hmm, what is the exact error message from mkfs here?

I guess that it would be a good idea to cat the temp file if mkswap
failed unexpectedly so that we can see the mkswap error message.

> Additionally, we'd better change the bs=1kB to bs=1024 in runltp file,
> since it will get page aligned block device for all arches.
> 
> The original LTP_DEV size is 102400000 bytes:
> 
> # grep 'bs=' runltp  -n
> 981:    dd if=/dev/zero of=${TMP}/test.img bs=1kB count=102400
> 
> 
> From dd manual:
> N and BYTES may be followed by the following multiplicative suffixes:
> c =1, w =2, b =512, kB  =1000,  K  =1024,  MB =1000*1000,  M  =1024*1024,
> xM  =M  GB  =1000*1000*1000,  G =1024*1024*1024, and so on for T, P, E, Z, Y.

Or change it to uppercase KB. Sounds good, feel free to send a patch.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Li Wang]

On Mon, Aug 15, 2016 at 02:39:59PM +0200, Cyril Hrubis wrote:
> Hi!
> > My version is:
> > 
> > # rpm -qf /sbin/mkswap 
> > util-linux-2.23.2-33.el7.ppc64
> > 
> > 
> > if (block_count) {
> > 	/* this silly user specified the number of blocks explicitly */
> > 		uint64_t blks = strtou64_or_err(block_count,
> > 					_("invalid block count argument"));
> > 		PAGES = blks / (pagesize / 1024);
> > 	}
> > 	sz = get_size(device_name);
> 
> There is no check here as well. The strtou64_or_err() just converts
> string to number, that's all it does. Then we do integer division, no
> checks at all.
> 
> > > Looking into util-linux-2.26.2 all it does with user specified number of
> > > blocks is:
> > > 
> > > ctl.npages = blks / (ctl.pagesize / 1024);
> > > 
> > > There is no check that blks is divideable by pagesize/1024.
> > 
> > I build util-linux-2.28 from upstrem source and also get the errors.
> > 
> > # mkswap -V
> > mkswap from util-linux 2.28.237-47bd89
> > 
> > # ./runltp -f commands
> > mkswap01 1 TINFO : Using test device LTP_DEV='/dev/loop0'
> > mkswap01 1 TFAIL : 'mkswap   /dev/loop0 ' failed, not expected.
> > mkswap01 2 TFAIL : 'mkswap   /dev/loop0 90000' failed, not expected.
> > mkswap01 3 TINFO : Can not do swapon on /dev/loop0.
> > mkswap01 3 TINFO : Device size specified by 'mkswap' greater than real size.
> > mkswap01 3 TINFO : Swapon failed expectedly.
> > mkswap01 3 TPASS : 'mkswap -f  /dev/loop0 110000' passed.
> 
> Hmm, what is the exact error message from mkfs here?
> 
> I guess that it would be a good idea to cat the temp file if mkswap
> failed unexpectedly so that we can see the mkswap error message.

Sorry for the vague description. To be honest, mkswap.c(util-linux) is innocent
here. The error maker is mkswap_verify() in mkswap01.sh(ltp) testcase.

The key point of problem is not only mkswap01.sh (ltp) doesn't guarantee
test-device(LTP_DEV) size is aligned to PAGE_SIZE on all arches, but also
it increse/decrease test-device size not aligned too.

# tail -15 mkswap01.sh
mkswap_test "" "" "$TST_DEVICE"
mkswap_test "" "" "$TST_DEVICE" "$((DEVICE_SIZE-10000))"     <---
mkswap_test "-f" "" "$TST_DEVICE" "$((DEVICE_SIZE+10000))"   <---
...


As we know that mkswap.c(util-linux) format test-device to aligned with its local
PAGE_SIZE by force. Therefore, the real 'filesize' of formated test-device will
be decreased in that phase. But final, the testcase just compare orignal 'filesize'
to 'diff'(swaptotal-after - swaptotal-before) and make conclusion to get PASS/FAIL.
That makes this test result is not reliable.

I temporally add some debug sentences in testcase to show the prblem:

mkswap01 1 TINFO : Using test device LTP_DEV='/dev/loop1'
mkswap01 1 TINFO : DEVICE_SIZE = 100000                  <--- should always give an aligned size here 
mkswap01 1 TINFO : ********DEBUG MSG: swapsize = 100000
mkswap01 1 TINFO : ********DEBUG MSG: diff = 99904
mkswap01 1 TINFO : ********DEBUG MSG: filesize = 99936
mkswap01 1 TFAIL : 'mkswap   /dev/loop1 ' failed, not expected.
mkswap01 2 TINFO : ********DEBUG MSG: swapsize = 90000   <--- here as well
mkswap01 2 TINFO : ********DEBUG MSG: diff = 89920
mkswap01 2 TINFO : ********DEBUG MSG: filesize = 89936
mkswap01 2 TFAIL : 'mkswap   /dev/loop1 90000' failed, not expected.
mkswap01 3 TINFO : ********DEBUG MSG: swapsize = 110000
mkswap01 3 TINFO : Can not do swapon on /dev/loop1.
mkswap01 3 TINFO : Device size specified by 'mkswap' greater than real size.
mkswap01 3 TINFO : Swapon failed expectedly.
mkswap01 3 TPASS : 'mkswap -f  /dev/loop1 110000' passed.


After fixing abvoe issues, we get test PASS correctly:

mkswap01 1 TINFO : Using test device LTP_DEV='/dev/loop1'
mkswap01 1 TINFO : DEVICE_SIZE = 102400
mkswap01 1 TINFO : ********DEBUG MSG: swapsize = 102400
mkswap01 1 TINFO : ********DEBUG MSG: diff = 102336
mkswap01 1 TINFO : ********DEBUG MSG: filesize = 102336
mkswap01 1 TPASS : 'mkswap   /dev/loop1 ' passed.
mkswap01 2 TINFO : ********DEBUG MSG: swapsize = 102336
mkswap01 2 TINFO : ********DEBUG MSG: diff = 102272
mkswap01 2 TINFO : ********DEBUG MSG: filesize = 102272
mkswap01 2 TPASS : 'mkswap   /dev/loop1 102336' passed.
mkswap01 3 TINFO : ********DEBUG MSG: swapsize = 102464
mkswap01 3 TINFO : Can not do swapon on /dev/loop1.
mkswap01 3 TINFO : Device size specified by 'mkswap' greater than real size.
mkswap01 3 TINFO : Swapon failed expectedly.
mkswap01 3 TPASS : 'mkswap -f  /dev/loop1 102464' passed.

> > Additionally, we'd better change the bs=1kB to bs=1024 in runltp file,
> > since it will get page aligned block device for all arches.
> > 
> > The original LTP_DEV size is 102400000 bytes:
> > 
> > # grep 'bs=' runltp  -n
> > 981:    dd if=/dev/zero of=${TMP}/test.img bs=1kB count=102400
> > 
> > 
> > From dd manual:
> > N and BYTES may be followed by the following multiplicative suffixes:
> > c =1, w =2, b =512, kB  =1000,  K  =1024,  MB =1000*1000,  M  =1024*1024,
> > xM  =M  GB  =1000*1000*1000,  G =1024*1024*1024, and so on for T, P, E, Z, Y.
> 
> Or change it to uppercase KB. Sounds good, feel free to send a patch.

Ok, thanks for reviewing.

Regards,
Li Wang

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Cyril Hrubis]

Hi!
> Sorry for the vague description. To be honest, mkswap.c(util-linux) is innocent
> here. The error maker is mkswap_verify() in mkswap01.sh(ltp) testcase.
> 
> The key point of problem is not only mkswap01.sh (ltp) doesn't guarantee
> test-device(LTP_DEV) size is aligned to PAGE_SIZE on all arches, but also
> it increse/decrease test-device size not aligned too.
> 
> # tail -15 mkswap01.sh
> mkswap_test "" "" "$TST_DEVICE"
> mkswap_test "" "" "$TST_DEVICE" "$((DEVICE_SIZE-10000))"     <---
> mkswap_test "-f" "" "$TST_DEVICE" "$((DEVICE_SIZE+10000))"   <---
> ...
> 
> 
> As we know that mkswap.c(util-linux) format test-device to aligned with its local
> PAGE_SIZE by force. Therefore, the real 'filesize' of formated test-device will
> be decreased in that phase. But final, the testcase just compare orignal 'filesize'
> to 'diff'(swaptotal-after - swaptotal-before) and make conclusion to get PASS/FAIL.
> That makes this test result is not reliable.

Now that makes sense.

But if that is the case we still have to align down the device size to
the page size at the start of the test. Since the device can also be
passed down to the runltp script as a parameter and we cannot make any
assumptions about its size.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Li Wang]

On Tue, Aug 16, 2016 at 02:15:52PM +0200, Cyril Hrubis wrote:
> Hi!
> > Sorry for the vague description. To be honest, mkswap.c(util-linux) is innocent
> > here. The error maker is mkswap_verify() in mkswap01.sh(ltp) testcase.
> > 
> > The key point of problem is not only mkswap01.sh (ltp) doesn't guarantee
> > test-device(LTP_DEV) size is aligned to PAGE_SIZE on all arches, but also
> > it increse/decrease test-device size not aligned too.
> > 
> > # tail -15 mkswap01.sh
> > mkswap_test "" "" "$TST_DEVICE"
> > mkswap_test "" "" "$TST_DEVICE" "$((DEVICE_SIZE-10000))"     <---
> > mkswap_test "-f" "" "$TST_DEVICE" "$((DEVICE_SIZE+10000))"   <---
> > ...
> > 
> > 
> > As we know that mkswap.c(util-linux) format test-device to aligned with its local
> > PAGE_SIZE by force. Therefore, the real 'filesize' of formated test-device will
> > be decreased in that phase. But final, the testcase just compare orignal 'filesize'
> > to 'diff'(swaptotal-after - swaptotal-before) and make conclusion to get PASS/FAIL.
> > That makes this test result is not reliable.
> 
> Now that makes sense.
> 
> But if that is the case we still have to align down the device size to
> the page size at the start of the test. Since the device can also be
> passed down to the runltp script as a parameter and we cannot make any
> assumptions about its size.

Hmm, yes, we have to face the problem.

A simple idea comes to my mind is that the loop device size should be
customized by specific testcase(mkswap01.sh).

That would be happy if function "tst_acquire_device() 'NUM'" can get
'NUM' MB block device for testing.

It means we have to modify another path which about tst_acquire_device().

What do you think? any proposals?

-----------
tst_acquire_device()
{
	local acq_dev_size=${1}

	if [ -z ${TST_TMPDIR} ]; then
		tst_brkm "Use 'tst_tmpdir' before 'tst_acquire_device'"
	fi

	if [ -n "${LTP_DEV}" ] && [ ! ${acq_dev_size} ]; then
		tst_resm TINFO "Using test device
		LTP_DEV='${LTP_DEV}'"
		if [ ! -b ${LTP_DEV} ]; then
			tst_brkm TBROK "${LTP_DEV} is not a block device"
		fi

		ROD_SILENT dd if=/dev/zero of="${LTP_DEV}" bs=1024 count=512 TST_DEVICE=${LTP_DEV}
		TST_DEVICE_FLAG=0
		return
	fi

	if [ ! ${acq_dev_size} ]; then
		ROD_SILENT dd if=/dev/zero of=test_dev.img bs=1024 count=153600
	else
		ROD_SILENT dd if=/dev/zero of=test_dev.img bs=1024 count=$((1024*$acq_dev_size))
	fi

	TST_DEVICE=$(losetup -f)
	if [ $? -ne 0 ]; then
		tst_brkm TBROK "Couldn't find free loop device"
	fi

	tst_resm TINFO "Found free device '${TST_DEVICE}'"

	ROD_SILENT losetup ${TST_DEVICE} test_dev.img

	TST_DEVICE_FLAG=1
}


Regards,
Li Wang

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Cyril Hrubis]

Hi!
> > Now that makes sense.
> > 
> > But if that is the case we still have to align down the device size to
> > the page size at the start of the test. Since the device can also be
> > passed down to the runltp script as a parameter and we cannot make any
> > assumptions about its size.
> 
> Hmm, yes, we have to face the problem.
> 
> A simple idea comes to my mind is that the loop device size should be
> customized by specific testcase(mkswap01.sh).
> 
> That would be happy if function "tst_acquire_device() 'NUM'" can get
> 'NUM' MB block device for testing.
> 
> It means we have to modify another path which about tst_acquire_device().

This is a bad idea for two reasons.

Fist one is that the reason the user passed block device to runltp may
be that the kernel does not support loop devices. In this case the test
would be skipped for no good reason.

The second is that there is another patch in flight that aims to add
minimal size parameter to the tst_acquire_device(). Which is much more
useful.

Why cannot we just simply take the size of the device at start of the
test, align it down to be the multiple of $PAGE_SIZE and use that as the
size for testing?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Li Wang]

On Wed, Aug 17, 2016 at 01:18:41PM +0200, Cyril Hrubis wrote:
> Hi!
> > > Now that makes sense.
> > > 
> > > But if that is the case we still have to align down the device size to
> > > the page size at the start of the test. Since the device can also be
> > > passed down to the runltp script as a parameter and we cannot make any
> > > assumptions about its size.
> > 
> > Hmm, yes, we have to face the problem.
> > 
> > A simple idea comes to my mind is that the loop device size should be
> > customized by specific testcase(mkswap01.sh).
> > 
> > That would be happy if function "tst_acquire_device() 'NUM'" can get
> > 'NUM' MB block device for testing.
> > 
> > It means we have to modify another path which about tst_acquire_device().
> 
> This is a bad idea for two reasons.
> 
> Fist one is that the reason the user passed block device to runltp may
> be that the kernel does not support loop devices. In this case the test
> would be skipped for no good reason.
> 
> The second is that there is another patch in flight that aims to add
> minimal size parameter to the tst_acquire_device(). Which is much more
> useful.
> 
> Why cannot we just simply take the size of the device at start of the
> test, align it down to be the multiple of $PAGE_SIZE and use that as the
> size for testing?

Ok, this sounds better. I misunderstand you point at first.

As this we can get the right(aligned) size of the device by add:

# cat mkswap01.sh
...
setup()
{
	...

	# Here get the size of the device and align it down to be the
	# multiple of $PAGE_SIZE and use that as the size for testing.
	device_real=`blockdev --getsize64 $TST_DEVICE`
	device_resi=$((${device_real} % ${PAGE_SIZE}))

	DEVICE_SIZE=$(((${device_real} - ${device_resi})/1024))
}

Am I right now?

Regards,
Li Wang

[LTP] [PATCH] mkswap: make the test device size is aligned to pagesize

[Cyril Hrubis]

Hi!
> Ok, this sounds better. I misunderstand you point at first.
> 
> As this we can get the right(aligned) size of the device by add:
> 
> # cat mkswap01.sh
> ...
> setup()
> {
> 	...
> 
> 	# Here get the size of the device and align it down to be the
> 	# multiple of $PAGE_SIZE and use that as the size for testing.
> 	device_real=`blockdev --getsize64 $TST_DEVICE`
> 	device_resi=$((${device_real} % ${PAGE_SIZE}))
> 
> 	DEVICE_SIZE=$(((${device_real} - ${device_resi})/1024))

You can also use and integer division, which is a bit shorter:

DEVICE_SIZE=$((($real_size/$PAGE_SIZE * $PAGE_SIZE)/1024))

-- 
Cyril Hrubis
chrubis@suse.cz