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

[Stefan Haberland <sth@linux.ibm.com>]

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.