Re: [PATCH AUTOSEL 5.2 149/171] s390/dasd: Make layout analysis ESE compatible

[Jan HÃppner]

On 19.07.19 09:47, Christian Borntraeger wrote:
> The comment is true for all stable versions.
> 
> This patch is part of a larger series that enables ESE volumes.
> I think it should not go alone as other patches like 
> 5e2b17e712cf s390/dasd: Add dynamic formatting support for ESE volumes
> are needed to actually work with ESE volumes.
> So I suggest to drop this patch.
> Jan, Stefan, do you agree?
> 

This patch is a requirement for ESE volumes to work and doesn't
add any value alone. I suggest to drop this patch as well.

Jan

> 
> 
> On 19.07.19 05:56, Sasha Levin wrote:
>> From: Jan HÃppner <hoeppner@xxxxxxxxxxxxx>
>>
>> [ Upstream commit ce6915f5343f5f2a2a937b683d8ffbf12dab3ad4 ]
>>
>> The disk layout and volume information of a DASD reside in the first two
>> tracks of cylinder 0. When a DASD is set online, currently the first
>> three tracks are read and analysed to confirm an expected layout.
>>
>> For CDL (Compatible Disk Layout) only count area data of the first track
>> is evaluated and checked against expected key and data lengths. For LDL
>> (Linux Disk Layout) the first and third track is evaluated. However,
>> an LDL formatted volume is expected to be in the same format across all
>> tracks. Checking the third track therefore doesn't have any more value
>> than checking any other track at random.
>>
>> Now, an Extent Space Efficient (ESE) DASD is initialised by only
>> formatting the first two tracks, as those tracks always contain all
>> information necessarry.
>>
>> Checking the third track on an ESE volume will therefore most likely
>> fail with a record not found error, as the third track will be empty.
>> This in turn leads to the device being recognised with a volume size of
>> 0. Attempts to write volume information on the first two tracks then
>> fail with "no space left on device" errors.
>>
>> Initialising the first three tracks for an ESE volume is not a viable
>> solution, because the third track is already a regular track and could
>> contain user data. With that there is potential for data corruption.
>>
>> Instead, always only analyse the first two tracks, as it is sufficiant
>> for both CDL and LDL, and allow ESE volumes to be recognised as well.
>>
>> Signed-off-by: Jan HÃppner <hoeppner@xxxxxxxxxxxxx>
>> Reviewed-by: Stefan Haberland <sth@xxxxxxxxxxxxx>
>> Signed-off-by: Vasily Gorbik <gor@xxxxxxxxxxxxx>
>> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
>> ---
>>  drivers/s390/block/dasd_eckd.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
>> index c09039eea707..c7aec1b44b7c 100644
>> --- a/drivers/s390/block/dasd_eckd.c
>> +++ b/drivers/s390/block/dasd_eckd.c
>> @@ -157,7 +157,7 @@ static const int sizes_trk0[] = { 28, 148, 84 };
>>  #define LABEL_SIZE 140
>>  
>>  /* head and record addresses of count_area read in analysis ccw */
>> -static const int count_area_head[] = { 0, 0, 0, 0, 2 };
>> +static const int count_area_head[] = { 0, 0, 0, 0, 1 };
>>  static const int count_area_rec[] = { 1, 2, 3, 4, 1 };
>>  
>>  static inline unsigned int
>> @@ -1823,8 +1823,8 @@ dasd_eckd_analysis_ccw(struct dasd_device *device)
>>  	if (IS_ERR(cqr))
>>  		return cqr;
>>  	ccw = cqr->cpaddr;
>> -	/* Define extent for the first 3 tracks. */
>> -	define_extent(ccw++, cqr->data, 0, 2,
>> +	/* Define extent for the first 2 tracks. */
>> +	define_extent(ccw++, cqr->data, 0, 1,
>>  		      DASD_ECKD_CCW_READ_COUNT, device, 0);
>>  	LO_data = cqr->data + sizeof(struct DE_eckd_data);
>>  	/* Locate record for the first 4 records on track 0. */
>> @@ -1843,9 +1843,9 @@ dasd_eckd_analysis_ccw(struct dasd_device *device)
>>  		count_data++;
>>  	}
>>  
>> -	/* Locate record for the first record on track 2. */
>> +	/* Locate record for the first record on track 1. */
>>  	ccw[-1].flags |= CCW_FLAG_CC;
>> -	locate_record(ccw++, LO_data++, 2, 0, 1,
>> +	locate_record(ccw++, LO_data++, 1, 0, 1,
>>  		      DASD_ECKD_CCW_READ_COUNT, device, 0);
>>  	/* Read count ccw. */
>>  	ccw[-1].flags |= CCW_FLAG_CC;
>> @@ -1967,7 +1967,7 @@ static int dasd_eckd_end_analysis(struct dasd_block *block)
>>  		}
>>  	}
>>  	if (i == 3)
>> -		count_area = &private->count_area[4];
>> +		count_area = &private->count_area[3];
>>  
>>  	if (private->uses_cdl == 0) {
>>  		for (i = 0; i < 5; i++) {
>>