Hi
On 06.04.2011, at 14:51, Stefan Haberland wrote:
> Hi,
>
> while we understand what you want to achieve, we have two issues with this patch:
Thanks for the reply at first!
> The assumption that a CDL disk has always 12 blocks per track is not correct. The number of blocks per track depends on the formatted blocksize and also on the hardware.
I've asked around and from what I've gathered there is no way to modify the 12 blocks/track piece in z/VM. I'm not sure if this is true - I'm not s390 expert myself - but at least I haven't seen a single disk that would behave differently.
Could you please tell me some way to format a dasd device so that it has a blocks/track value different from 12? Also, do people actually do this?
> On the one side you add two parameters to distinguish between CDL and LDL but you do not allow to distinguish between ECKD and FBA. The difference between CDL and LDL could be deduced from the volume label itself but the ECKD/FBA distinction is important because the label block position depends on this (info->label_block).
> The case of FBA devices seems important to us because people may want to use images they can copy from or to z/VM emulated FBA devices on SCSI disks (EDEV).
Yes, either case does make sense. You might also want to be able to copy an image from a dasd device over to a SCSI disk. So what would you suggest? Should I add a force_fba flag as well?
>
> Am 31.03.2011 17:59, schrieb Alexander Graf:
>> Currently we can only use the DASD partition label on DASD disks. When running
>> a VM in KVM, we only have virtio disks available though. In order to still be
>> able to use DASD formatted disks, we need a flag that allows us to force enable
>> detection of the DASD partition label. This patch implements parameters to
>> achieve this.
>>
>> For example, to activate the label code to force devices "vda" and "vdb" to be
>> detected as CDL style label, add "ibm.dasd_force_cdl=vda,vdb" to your kernel
>> command line. A respective parameter is also added for the LDL label type.
>>
>> Signed-off-by: Alexander Graf<agraf@xxxxxxx>
>> ---
>> fs/partitions/ibm.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/partitions/ibm.c b/fs/partitions/ibm.c
>> index d513a07..5a4b9f6 100644
>> --- a/fs/partitions/ibm.c
>> +++ b/fs/partitions/ibm.c
>> @@ -17,6 +17,30 @@
>> #include "check.h"
>> #include "ibm.h"
>>
>> +static char *dasd_force_ldl[4];
>> +static char *dasd_force_cdl[4];
>> +
>> +module_param_array(dasd_force_ldl, charp, NULL, 0);
>> +MODULE_PARM_DESC(dasd_force_ldl, "force a block device to be detected as DASD "
>> + "(LDL format)");
>> +module_param_array(dasd_force_cdl, charp, NULL, 0);
>> +MODULE_PARM_DESC(dasd_force_cdl, "force a block device to be detected as DASD "
>> + "(CDL format)");
>> +
>> +static inline bool match_array(char **arr, const char *match)
>> +{
>> + if (!arr)
>> + return false;
>> +
>> + while (*arr) {
>> + if (!strcmp(*arr, match))
>> + return true;
>> + arr++;
>> + }
>> +
>> + return false;
>> +}
>> +
>> /*
>> * compute the block number from a
>> * cyl-cyl-head-head structure
>> @@ -76,6 +100,7 @@ int ibm_partition(struct parsed_partitions *state)
>> Sector sect;
>> sector_t labelsect;
>> char tmp[64];
>> + bool fake_cdl = false;
>>
>> res = 0;
>> blocksize = bdev_logical_block_size(bdev);
>> @@ -95,10 +120,31 @@ int ibm_partition(struct parsed_partitions *state)
>> if (label == NULL)
>> goto out_nolab;
>>
>> - if (ioctl_by_bdev(bdev, BIODASDINFO2, (unsigned long)info) != 0 ||
>> - ioctl_by_bdev(bdev, HDIO_GETGEO, (unsigned long)geo) != 0)
>> + if (match_array(dasd_force_ldl, bdev->bd_disk->disk_name)) {
>> + memset(info, 0, sizeof(dasd_information2_t));
>> + info->format = DASD_FORMAT_LDL;
>> + info->label_block = 2;
>> + } else if (match_array(dasd_force_cdl, bdev->bd_disk->disk_name)) {
>> + memset(info, 0, sizeof(dasd_information2_t));
>> + info->format = DASD_FORMAT_CDL;
>> + info->label_block = 2;
>> + fake_cdl = true;
>> + } else if (ioctl_by_bdev(bdev, BIODASDINFO2, (unsigned long)info) != 0) {
>> + goto out_freeall;
>> + }
>> +
>> + if (ioctl_by_bdev(bdev, HDIO_GETGEO, (unsigned long)geo) != 0)
>> goto out_freeall;
>>
>> + if (fake_cdl) {
>> + memset(geo, 0, sizeof(dasd_information2_t));
> geo is from type struct hd_geometry
Yikes. Not sure what I was thinking. Looking at the code it'd be the best to simply remove that line.
>> + /* CDL disks always are on 15/12 layout, so calculate over */
>> + geo->cylinders = (geo->heads * geo->sectors * geo->cylinders)
>> + / (15 * 12);
> geo->cylinders is alway 0 if you calculate with values from geo set to 0
Yeah - the whole memset is suspicious.
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
