lib: tst_device: Allow more control over the device size

There is actually no reason for lower limit on the device size, and we
can safely allow the tests to request smaller device than the default
hardcoded in the library. The backing file is preallocated without
actually writing to it as long as the underlying filesystem supports it
so the speedup will be minimal if measurable but we will at least spare
some space which needs to be reserved on the filesystem which is still a
good thing.

The test may end up with a device that is bigger than the requsted size
in a case that a real device was passed to the LTP for the testrun.  So
tests should be able to cope with that and that's also the reason why
the turning knob is still called dev_min_size.

Also currently we use the dev_min_size only to increase the device size
for a few tests so this change is safe and cannot break anything.

CC: Joerg Vehlow <lkml@jv-coder.de>
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 lib/tst_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Fri, Jul 30, 2021 at 9:32 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> There is actually no reason for lower limit on the device size, and we
> can safely allow the tests to request smaller device than the default
> hardcoded in the library. The backing file is preallocated without
> actually writing to it as long as the underlying filesystem supports it
> so the speedup will be minimal if measurable but we will at least spare
> some space which needs to be reserved on the filesystem which is still a
> good thing.
>
> The test may end up with a device that is bigger than the requsted size
> in a case that a real device was passed to the LTP for the testrun.  So
> tests should be able to cope with that and that's also the reason why
> the turning knob is still called dev_min_size.
>
> Also currently we use the dev_min_size only to increase the device size
> for a few tests so this change is safe and cannot break anything.
>
> CC: Joerg Vehlow <lkml@jv-coder.de>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
>

Reviewed-by: Li Wang <liwang@redhat.com>

Hi Cyril,

On 7/30/2021 3:31 PM, Cyril Hrubis wrote:
> There is actually no reason for lower limit on the device size, and we
> can safely allow the tests to request smaller device than the default
> hardcoded in the library. The backing file is preallocated without
> actually writing to it as long as the underlying filesystem supports it
> so the speedup will be minimal if measurable but we will at least spare
> some space which needs to be reserved on the filesystem which is still a
> good thing.
>
> The test may end up with a device that is bigger than the requsted size
> in a case that a real device was passed to the LTP for the testrun.  So
> tests should be able to cope with that and that's also the reason why
> the turning knob is still called dev_min_size.
>
> Also currently we use the dev_min_size only to increase the device size
> for a few tests so this change is safe and cannot break anything.
>
> CC: Joerg Vehlow <lkml@jv-coder.de>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>   lib/tst_device.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index c91c6cd55..4ef802c41 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -300,7 +300,7 @@ const char *tst_acquire_device__(unsigned int size)
>   	unsigned int acq_dev_size;
>   	uint64_t ltp_dev_size;
>   
> -	acq_dev_size = MAX(size, DEV_SIZE_MB);
> +	acq_dev_size = size ? size : DEV_SIZE_MB;
>   
>   	dev = getenv("LTP_DEV");
>   
This is not enough. tst_acquire_device__ calls tst_acquire_loop_device, 
that again has MAX(size, DEV_SIZE_MB).
But it should be sage to substitute it for size ? size : DEV_SIZE_MB as 
well.

Joerg

Hi!
> This is not enough. tst_acquire_device__ calls tst_acquire_loop_device, 
> that again has MAX(size, DEV_SIZE_MB).
> But it should be sage to substitute it for size ? size : DEV_SIZE_MB as 
> well.

Right, that was the old API function to get loop devices which was
called from old API testcases. Looks like there are no old API tests
that work with loop devices anymore, so this function should be removed
from the public API as well. I will send v2 patchset.

Hi!
> > This is not enough. tst_acquire_device__ calls tst_acquire_loop_device, 
> > that again has MAX(size, DEV_SIZE_MB).
> > But it should be sage to substitute it for size ? size : DEV_SIZE_MB as 
> > well.
> 
> Right, that was the old API function to get loop devices which was
> called from old API testcases. Looks like there are no old API tests
> that work with loop devices anymore, so this function should be removed
> from the public API as well. I will send v2 patchset.

Uff, that was tst_acquire_device_() not tst_acquire_loop_device() and
tst_acquire_device_() is still in use.

I'm really looking forward to a day where we can finally remove the old
API from the library...

Hi,

On 8/2/2021 1:43 PM, Cyril Hrubis wrote:
>>> This is not enough. tst_acquire_device__ calls tst_acquire_loop_device,
>>> that again has MAX(size, DEV_SIZE_MB).
>>> But it should be sage to substitute it for size ? size : DEV_SIZE_MB as
>>> well.
>> Right, that was the old API function to get loop devices which was
>> called from old API testcases. Looks like there are no old API tests
>> that work with loop devices anymore, so this function should be removed
>> from the public API as well. I will send v2 patchset.
> Uff, that was tst_acquire_device_() not tst_acquire_loop_device() and
> tst_acquire_device_() is still in use.
>
> I'm really looking forward to a day where we can finally remove the old
> API from the library...
The usage of foo foo_ and foo__ does not really help in reading the code :)
There could also be some logic errors hiding, e.g. 
tst_acquire_loop_device should probably not default to DEV_SIZE_MB at all.
The caller should be responsible for finding a correct size and the two 
users of this function (tst_device [the binary] and 
tst_acquire_device__) do pass a concrete value for size.

Joerg

Hi!
> The usage of foo foo_ and foo__ does not really help in reading the code :)
> There could also be some logic errors hiding, e.g. 
> tst_acquire_loop_device should probably not default to DEV_SIZE_MB at all.
> The caller should be responsible for finding a correct size and the two 
> users of this function (tst_device [the binary] and 
> tst_acquire_device__) do pass a concrete value for size.

Actually the tst_device binary does not pass a concrete size unless the
shell code that calls it passes an optional parameter, so the fallback
to DEV_SIZE_MB if size == 0 has to stay in the double underscore
function. I will send a v2 that just replaces the second occurence of
MAX() in tst_device.c for now as it looks to me that any singificant
cleanup would require complete redesing of the interface and quite
possibly rewrite of the last 16 tests that use the old API as a
pre-requisite.

Hi Cyril,

On 8/2/2021 2:14 PM, Cyril Hrubis wrote:
> Hi!
>> The usage of foo foo_ and foo__ does not really help in reading the code :)
>> There could also be some logic errors hiding, e.g.
>> tst_acquire_loop_device should probably not default to DEV_SIZE_MB at all.
>> The caller should be responsible for finding a correct size and the two
>> users of this function (tst_device [the binary] and
>> tst_acquire_device__) do pass a concrete value for size.
> Actually the tst_device binary does not pass a concrete size unless the
> shell code that calls it passes an optional parameter, so the fallback
> to DEV_SIZE_MB if size == 0 has to stay in the double underscore
> function. I will send a v2 that just replaces the second occurence of
> MAX() in tst_device.c for now as it looks to me that any singificant
> cleanup would require complete redesing of the interface and quite
> possibly rewrite of the last 16 tests that use the old API as a
> pre-requisite.
I think we maximized confusion.
I was not arguing about the defaulting to DEV_SIZE_MB in the double 
underscore function, but about the defaulting in the 
tst_acquire_loop_device function. This function is never called with 
size=0, because the call is either from the double underscore function, 
that defaults to DEV_SIZE_MB or from the tst_device binary, that only 
calls tst_acquire_loop_device if the 3 argument version (tst_device 
acquire [size [filename]]) is used and size is not allowed to be 0 in 
that case.

Hi!
> >> The usage of foo foo_ and foo__ does not really help in reading the code :)
> >> There could also be some logic errors hiding, e.g.
> >> tst_acquire_loop_device should probably not default to DEV_SIZE_MB at all.
> >> The caller should be responsible for finding a correct size and the two
> >> users of this function (tst_device [the binary] and
> >> tst_acquire_device__) do pass a concrete value for size.
> > Actually the tst_device binary does not pass a concrete size unless the
> > shell code that calls it passes an optional parameter, so the fallback
> > to DEV_SIZE_MB if size == 0 has to stay in the double underscore
> > function. I will send a v2 that just replaces the second occurence of
> > MAX() in tst_device.c for now as it looks to me that any singificant
> > cleanup would require complete redesing of the interface and quite
> > possibly rewrite of the last 16 tests that use the old API as a
> > pre-requisite.
> I think we maximized confusion.
> I was not arguing about the defaulting to DEV_SIZE_MB in the double 
> underscore function, but about the defaulting in the 
> tst_acquire_loop_device function. This function is never called with 
> size=0, because the call is either from the double underscore function, 
> that defaults to DEV_SIZE_MB or from the tst_device binary, that only 
> calls tst_acquire_loop_device if the 3 argument version (tst_device 
> acquire [size [filename]]) is used and size is not allowed to be 0 in 
> that case.

Well it's true that at the moment all callers pass non-zero size to the
tst_device binary but any shell test can pass down 0 to the call in
order to get the default size which is large enough for all possible
filesystems.