Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Al Viro]

On Sat, Apr 06, 2024 at 05:09:19PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Avoid to access bd_inode directly, prepare to remove bd_inode from
> block_devcie.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  drivers/s390/block/dasd_ioctl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
> index 7e0ed7032f76..c1201590f343 100644
> --- a/drivers/s390/block/dasd_ioctl.c
> +++ b/drivers/s390/block/dasd_ioctl.c
> @@ -215,8 +215,9 @@ dasd_format(struct dasd_block *block, struct format_data_t *fdata)
>  	 * enabling the device later.
>  	 */
>  	if (fdata->start_unit == 0) {
> -		block->gdp->part0->bd_inode->i_blkbits =
> -			blksize_bits(fdata->blksize);
> +		rc = set_blocksize(block->gdp->part0, fdata->blksize);

Could somebody (preferably s390 folks) explain what is going on in
dasd_format()?  The change in this commit is *NOT* an equivalent
transformation - mainline does not evict the page cache of device.

Is that
	* intentional behaviour in mainline version, possibly broken
by this patch
	* a bug in mainline accidentally fixed by this patch
	* something else?

And shouldn't there be an exclusion between that and having a filesystem
on a partition of that disk currently mounted?

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Alexander Gordeev]

On Tue, Apr 16, 2024 at 02:35:55AM +0100, Al Viro wrote:
> >  drivers/s390/block/dasd_ioctl.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
> > index 7e0ed7032f76..c1201590f343 100644
> > --- a/drivers/s390/block/dasd_ioctl.c
> > +++ b/drivers/s390/block/dasd_ioctl.c
> > @@ -215,8 +215,9 @@ dasd_format(struct dasd_block *block, struct format_data_t *fdata)
> >  	 * enabling the device later.
> >  	 */
> >  	if (fdata->start_unit == 0) {
> > -		block->gdp->part0->bd_inode->i_blkbits =
> > -			blksize_bits(fdata->blksize);
> > +		rc = set_blocksize(block->gdp->part0, fdata->blksize);
> 
> Could somebody (preferably s390 folks) explain what is going on in
> dasd_format()?  The change in this commit is *NOT* an equivalent
> transformation - mainline does not evict the page cache of device.
> 
> Is that
> 	* intentional behaviour in mainline version, possibly broken
> by this patch
> 	* a bug in mainline accidentally fixed by this patch
> 	* something else?
> 
> And shouldn't there be an exclusion between that and having a filesystem
> on a partition of that disk currently mounted?

CC-ing Stefan and Jan.

Thanks!

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Stefan Haberland]

Am 16.04.24 um 10:47 schrieb Alexander Gordeev:
> On Tue, Apr 16, 2024 at 02:35:55AM +0100, Al Viro wrote:
>>>   drivers/s390/block/dasd_ioctl.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
>>> index 7e0ed7032f76..c1201590f343 100644
>>> --- a/drivers/s390/block/dasd_ioctl.c
>>> +++ b/drivers/s390/block/dasd_ioctl.c
>>> @@ -215,8 +215,9 @@ dasd_format(struct dasd_block *block, struct format_data_t *fdata)
>>>   	 * enabling the device later.
>>>   	 */
>>>   	if (fdata->start_unit == 0) {
>>> -		block->gdp->part0->bd_inode->i_blkbits =
>>> -			blksize_bits(fdata->blksize);
>>> +		rc = set_blocksize(block->gdp->part0, fdata->blksize);
>> Could somebody (preferably s390 folks) explain what is going on in
>> dasd_format()?  The change in this commit is *NOT* an equivalent
>> transformation - mainline does not evict the page cache of device.
>>
>> Is that
>> 	* intentional behaviour in mainline version, possibly broken
>> by this patch
>> 	* a bug in mainline accidentally fixed by this patch
>> 	* something else?
>>
>> And shouldn't there be an exclusion between that and having a filesystem
>> on a partition of that disk currently mounted?
> CC-ing Stefan and Jan.
>
> Thanks!

Hi,
from my point of view this was an equivalent transformation.

set_blocksize() does basically also set i_blkbits like it was before.
The dasd_format ioctl does only work on a disabled device. To achieve this
all partitions need to be unmounted.
The tooling also refuses to work on disks actually in use.

So there should be no page cache to evict.

The comment above this code says:

/* Since dasdfmt keeps the device open after it was disabled,
  * there still exists an inode for this device.
  * We must update i_blkbits, otherwise we might get errors when
  * enabling the device later.
  */

This is the reason for updating i_blkbits.

However, I get your point to question the code itself.

Honestly this code exists for many years and I can not tell if the
circumstances of the comment have changed in between somehow.
A quick test without this code did not show any change or errors but
there might be corner cases I am missing.

Maybe you can give a hint if this makes any sense from your point of view.

Thanks,
Stefan

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Al Viro]

On Wed, Apr 17, 2024 at 02:47:14PM +0200, Stefan Haberland wrote:

> set_blocksize() does basically also set i_blkbits like it was before.
> The dasd_format ioctl does only work on a disabled device. To achieve this
> all partitions need to be unmounted.
> The tooling also refuses to work on disks actually in use.
> 
> So there should be no page cache to evict.

You mean this?
        if (base->state != DASD_STATE_BASIC) {
                pr_warn("%s: The DASD cannot be formatted while it is enabled\n",
                        dev_name(&base->cdev->dev));
                return -EBUSY;
        }  

OK, but what would prevent dasd_ioctl_disable() from working while
disk is in use?  And I don't see anything that would evict the
page cache in dasd_ioctl_disable() either, actually...

What am I missing here?

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Al Viro]

On Sun, Apr 28, 2024 at 07:58:23PM +0100, Al Viro wrote:
> On Wed, Apr 17, 2024 at 02:47:14PM +0200, Stefan Haberland wrote:
> 
> > set_blocksize() does basically also set i_blkbits like it was before.
> > The dasd_format ioctl does only work on a disabled device. To achieve this
> > all partitions need to be unmounted.
> > The tooling also refuses to work on disks actually in use.
> > 
> > So there should be no page cache to evict.
> 
> You mean this?
>         if (base->state != DASD_STATE_BASIC) {
>                 pr_warn("%s: The DASD cannot be formatted while it is enabled\n",
>                         dev_name(&base->cdev->dev));
>                 return -EBUSY;
>         }  
> 
> OK, but what would prevent dasd_ioctl_disable() from working while
> disk is in use?  And I don't see anything that would evict the
> page cache in dasd_ioctl_disable() either, actually...
> 
> What am I missing here?

BTW, you are updating block size according to new device size, before
        rc = base->discipline->format_device(base, fdata, 1);
	if (rc == -EAGAIN)
		rc = base->discipline->format_device(base, fdata, 0);
Unless something very unidiomatic is going on, this attempt to
format might fail...

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Stefan Haberland]

Am 29.04.24 um 01:23 schrieb Al Viro:
> On Sun, Apr 28, 2024 at 07:58:23PM +0100, Al Viro wrote:
>> On Wed, Apr 17, 2024 at 02:47:14PM +0200, Stefan Haberland wrote:
>>
>>> set_blocksize() does basically also set i_blkbits like it was before.
>>> The dasd_format ioctl does only work on a disabled device. To achieve this
>>> all partitions need to be unmounted.
>>> The tooling also refuses to work on disks actually in use.
>>>
>>> So there should be no page cache to evict.
>> You mean this?
>>          if (base->state != DASD_STATE_BASIC) {
>>                  pr_warn("%s: The DASD cannot be formatted while it is enabled\n",
>>                          dev_name(&base->cdev->dev));
>>                  return -EBUSY;
>>          }
>>
>> OK, but what would prevent dasd_ioctl_disable() from working while
>> disk is in use?  And I don't see anything that would evict the
>> page cache in dasd_ioctl_disable() either, actually...
>>
>> What am I missing here?

Thank you for your input.
Let me provide some more insides how it is intended to work.
Maybe there is something we should improve.

This whole code is basically intended to be used by the dasdfmt tool.

For the dasdfmt tool and the dasd_format ioctl we are talking about DASD
ECKD devices.
An important note: for those devices a partition has to be used to access
the disk because the first tracks of the disks are not safe to store user
data. A partition has to be created by fdasd.

A disk in use has the state DASD_STATE_ONLINE.
To format a device the dasdfmt tool has to be called, it does the
following:

The dasdfmt tool checks if the disk is actually in use and refuses to
work on an 'in use' DASD.
So for example a partition that was in use has to be unmounted first.

Afterwards it does the following calls:

BIODASDDISABLE
  - to disable the device and prevent further usage
  - sets the disk in state DASD_STATE_BASIC
BIODASDFMT
  - does the actual formatting
  - checks if the disk is in state DASD_STATE_BASIC (if BIODASDDISABLE was
    called before)
  - this ioctl is usually called multiple times to format smaller parts of
    the disk each time
  - in the first call to this ioctl the first track (track 0) is
    invalidated (basically wiped out) and format_data_t.intensity equals
DASD_FMT_INT_INVAL
  - the last step is to finally format the first track to indicate a
    successful formatting of the whole disk
BIODASDENABLE
  - to enable the disk again for general usage
  - sets the disk to state DASD_STATE_ONLINE again
  - NOTE: a disabled device refuses an open call, so the tooling needs to
    keep the file descriptor open.

So the assumption in this processing is that a possibly used page cache is
evicted when removing the partition from actual usage (e.g. unmounting, ..).

While writing this I get to the point that it might not be the best idea to
rely on proper tool handling only and it might be a good idea to check for
an open count in BIODASDDISABLE as well so that the ioctls itself are safe
to use. (While it does not make a lot sense to use them alone.)
My assumption was that this is already done but obviously it isn't.

> BTW, you are updating block size according to new device size, before
>          rc = base->discipline->format_device(base, fdata, 1);
> 	if (rc == -EAGAIN)
> 		rc = base->discipline->format_device(base, fdata, 0);
> Unless something very unidiomatic is going on, this attempt to
> format might fail...

This is true. I guess the idea here was that the actual formatting of
track 0 is done last after the whole disk was successfully formatted and
everything went fine.
But actually also the invalidation of the first track would do this here.

So we should not only move this after the format_device call but we should
also add a check for DASD_FMT_INT_INVAL which is the first step in the
whole formatting.


My current conclusion would be that this patch itself is fine as is but I
should submit patches later to address the findings in this discussion.

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Al Viro]

On Mon, Apr 29, 2024 at 04:41:19PM +0200, Stefan Haberland wrote:

> The dasdfmt tool checks if the disk is actually in use and refuses to
> work on an 'in use' DASD.
> So for example a partition that was in use has to be unmounted first.

Hmm...  How is that check done?  Does it open device exclusive?

Re: [PATCH vfs.all 15/26] s390/dasd: use bdev api in dasd_format()

[Stefan Haberland]

Am 30.04.24 um 02:30 schrieb Al Viro:
> On Mon, Apr 29, 2024 at 04:41:19PM +0200, Stefan Haberland wrote:
>
>> The dasdfmt tool checks if the disk is actually in use and refuses to
>> work on an 'in use' DASD.
>> So for example a partition that was in use has to be unmounted first.
> Hmm...  How is that check done?  Does it open device exclusive?
>

No, it just checks the open_count gathered from the driver through 
another ioctl.

And yes, of course there is a race in this check that between gathering 
the data
and disabling the device it could be opened.