| Message ID | 20200430111754.98508-2-sth@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remove ioclt_by_bdev from DASD | expand |
On Thu, Apr 30, 2020 at 01:17:54PM +0200, Stefan Haberland wrote: > Remove the calls to ioctl_by_bdev from the DASD partition detection code > to enable the removal of the specific code. > > To do so reuse the gendisk private_data pointer and not only provide a > pointer to the devmap but provide a new structure containing a pointer > to the devmap as well as all required information for the partition > detection. This makes it independent from the dasd_information2_t > structure. I think sharing the data structure in private data is pretty dangerous. In the meantime I thought of another idea - the partition code could do a symbol_get of a symbol exported by the dasd driver and use that to query the information.
Am 30.04.20 um 15:13 schrieb Christoph Hellwig: > On Thu, Apr 30, 2020 at 01:17:54PM +0200, Stefan Haberland wrote: >> Remove the calls to ioctl_by_bdev from the DASD partition detection code >> to enable the removal of the specific code. >> >> To do so reuse the gendisk private_data pointer and not only provide a >> pointer to the devmap but provide a new structure containing a pointer >> to the devmap as well as all required information for the partition >> detection. This makes it independent from the dasd_information2_t >> structure. > I think sharing the data structure in private data is pretty dangerous. Thought of this as well. This is why I check for the major number before I use the private pointer to reference the data structure. Thought this would be enough checking. Do you think this is not sufficient? > In the meantime I thought of another idea - the partition code could > do a symbol_get of a symbol exported by the dasd driver and use that > to query the information. Then I would need to export a lot of DASD internal structures to be available in thepartition detection code if I would like to walk down our device map to findthe corresponding device for example. Not sure if this is that easy.
Am 30.04.20 um 16:02 schrieb Stefan Haberland: > Am 30.04.20 um 15:13 schrieb Christoph Hellwig: >> On Thu, Apr 30, 2020 at 01:17:54PM +0200, Stefan Haberland wrote: >>> Remove the calls to ioctl_by_bdev from the DASD partition detection code >>> to enable the removal of the specific code. >>> >>> To do so reuse the gendisk private_data pointer and not only provide a >>> pointer to the devmap but provide a new structure containing a pointer >>> to the devmap as well as all required information for the partition >>> detection. This makes it independent from the dasd_information2_t >>> structure. >> I think sharing the data structure in private data is pretty dangerous. > Thought of this as well. This is why I check for the major number before I > use the private pointer to reference the data structure. Thought this would > be enough checking. > Do you think this is not sufficient? > >> In the meantime I thought of another idea - the partition code could >> do a symbol_get of a symbol exported by the dasd driver and use that >> to query the information. > Then I would need to export a lot of DASD internal structures to be > available > in thepartition detection code if I would like to walk down our device > map to > findthe corresponding device for example. Not sure if this is that easy. I did some additional research on this. What I could imagine: The gendisk->private_data pointer currently contains a pointer to the dasd_devmap structure. This one is also reachable by iterating over an exported dasd_hashlist. So I could export the dasd_hashlist symbol, iterate over it and try to find the dasd_devmap pointer I have from the gendisk->private_data pointer. This would ensure that the gendisk belongs to the DASD driver and I could use the additional information that is somehow reachable through the gendisk->private_data pointer. But again, I am not sure if this additional code and effort is needed. From my point of view checking the gendisk->major for DASD_MAJOR is OK to ensure that the device belongs to the DASD driver. What do you think?
On Mon, May 04, 2020 at 10:45:33AM +0200, Stefan Haberland wrote: > > findthe corresponding device for example. Not sure if this is that easy. > > I did some additional research on this. > What I could imagine: > > The gendisk->private_data pointer currently contains a pointer to > the dasd_devmap structure. This one is also reachable by iterating > over an exported dasd_hashlist. > So I could export the dasd_hashlist symbol, iterate over it and try > to find the dasd_devmap pointer I have from the gendisk->private_data > pointer. > This would ensure that the gendisk belongs to the DASD driver and I > could use the additional information that is somehow reachable through > the gendisk->private_data pointer. > > But again, I am not sure if this additional code and effort is needed. > From my point of view checking the gendisk->major for DASD_MAJOR is > OK to ensure that the device belongs to the DASD driver. With CONFIG_DEBUG_BLOCK_EXT_DEVT you can't rely on major numbers. And compared to all the complications I think the biodasdinfo method is the least of all those evils. Jens, any opinion?
Am 05.05.20 um 14:44 schrieb Christoph Hellwig: > On Mon, May 04, 2020 at 10:45:33AM +0200, Stefan Haberland wrote: >>> findthe corresponding device for example. Not sure if this is that easy. >> I did some additional research on this. >> What I could imagine: >> >> The gendisk->private_data pointer currently contains a pointer to >> the dasd_devmap structure. This one is also reachable by iterating >> over an exported dasd_hashlist. >> So I could export the dasd_hashlist symbol, iterate over it and try >> to find the dasd_devmap pointer I have from the gendisk->private_data >> pointer. >> This would ensure that the gendisk belongs to the DASD driver and I >> could use the additional information that is somehow reachable through >> the gendisk->private_data pointer. >> >> But again, I am not sure if this additional code and effort is needed. >> From my point of view checking the gendisk->major for DASD_MAJOR is >> OK to ensure that the device belongs to the DASD driver. > With CONFIG_DEBUG_BLOCK_EXT_DEVT you can't rely on major numbers. OK, thanks for the hint.I did not have this in mind. And I still have to look up how this is working at all. But isn't this only a real issue for devices with more than 16 minors or partitions? So it should not be a problem for DASDs with our limit of 3 partitions and the fixed amount of minors, right? Just tested with CONFIG_DEBUG_BLOCK_EXT_DEVT enabled and about 1000 unlabeled devices. Did not see an issue. While I see the SCSI devices with MAJOR 259 and quite a random MINOR all the DASD devices keep their MAJOR 94 and ascending MINOR. > > And compared to all the complications I think the biodasdinfo method > is the least of all those evils. Are you talking about your first patch suggestion?Then I disagree. I still do not like to force the driver to be built in if there is an alternative. If the major number is an issue also for DASD device than I prefer to implement the devmap lookup to ensure that the device belongs to the DASD driver. But I am open to alternatives as well. Side note: I am planning to deprecate the implicit DASD partition for unlabeled devices so we might get rid of this stuff at all at some point in time. We just have to discuss how this might be done properly. > Jens, any opinion?
On Tue, May 05, 2020 at 05:09:56PM +0200, Stefan Haberland wrote: > OK, thanks for the hint.I did not have this in mind. And I still have > to look up how this is working at all. > But isn't this only a real issue for devices with more than 16 minors > or partitions? So it should not be a problem for DASDs with our limit > of 3 partitions and the fixed amount of minors, right? > > Just tested with CONFIG_DEBUG_BLOCK_EXT_DEVT enabled and about 1000 > unlabeled devices. Did not see an issue. > > While I see the SCSI devices with MAJOR 259 and quite a random MINOR > all the DASD devices keep their MAJOR 94 and ascending MINOR. Looks like it only changes the minors, and not the majors. Still checking for major and relying on a shared structure define in different places just doesn't look maintainable. > > And compared to all the complications I think the biodasdinfo method > > is the least of all those evils. > > Are you talking about your first patch suggestion?Then I disagree. > I still do not like to force the driver to be built in if there is an > alternative. No, I mean the series that I actually sent out: https://lkml.org/lkml/2020/4/21/66 https://lkml.org/lkml/2020/4/21/68 https://lkml.org/lkml/2020/4/21/69
Am 06.05.20 um 06:52 schrieb Christoph Hellwig: > > No, I mean the series that I actually sent out: > > https://lkml.org/lkml/2020/4/21/66 > https://lkml.org/lkml/2020/4/21/68 > https://lkml.org/lkml/2020/4/21/69 OK, just thought again about your suggestion and also that you have already been talking about a symbol lookup I just have written a prototype that took your first two patches as base, exports the symbol of dasd_biodasdinfo and in ibm.c I do a kallsyms_lookup_name("dasd_biodasdinfo"). So I would not have to define a structure twice or rely on MAJORs. Also we would not have to define an own file operation only for DASD devices. What do you think about this? If you agree I will polish the patches, test them and send them for review.
On Thu, May 07, 2020 at 05:22:28PM +0200, Stefan Haberland wrote: > OK, just thought again about your suggestion and also that > you have already been talking about a symbol lookup I just > have written a prototype that took your first two patches > as base, exports the symbol of dasd_biodasdinfo and in > ibm.c I do a kallsyms_lookup_name("dasd_biodasdinfo"). > > So I would not have to define a structure twice or rely on > MAJORs. Also we would not have to define an own file > operation only for DASD devices. > > What do you think about this? If you agree I will polish > the patches, test them and send them for review. How do you figure out a given gendisk is a DASD device? I guess dasd_biodasdinfo could just check the block_device_operations pointer first thing. That's still a little ugly, but seems the least bad idea so far, so please at least post it for discussion.
Am 07.05.20 um 17:29 schrieb Christoph Hellwig: > On Thu, May 07, 2020 at 05:22:28PM +0200, Stefan Haberland wrote: >> OK, just thought again about your suggestion and also that >> you have already been talking about a symbol lookup I just >> have written a prototype that took your first two patches >> as base, exports the symbol of dasd_biodasdinfo and in >> ibm.c I do a kallsyms_lookup_name("dasd_biodasdinfo"). >> >> So I would not have to define a structure twice or rely on >> MAJORs. Also we would not have to define an own file >> operation only for DASD devices. >> >> What do you think about this? If you agree I will polish >> the patches, test them and send them for review. > How do you figure out a given gendisk is a DASD device? I guess > dasd_biodasdinfo could just check the block_device_operations > pointer first thing. That's still a little ugly, but seems the > least bad idea so far, so please at least post it for discussion. Not checked till now. I just was thinking about the basicapproach. I could either check the block_device_operations like you suggested or I could verify that the gendisk pointer is already in our devmap and therefor belongs to the DASD driver. Will post a patch shortly.
On Thu, May 07, 2020 at 05:43:40PM +0200, Stefan Haberland wrote: > Not checked till now. I just was thinking about the basicapproach. > > I could either check the block_device_operations like you suggested > or I could verify that the gendisk pointer is already in our devmap > and therefor belongs to the DASD driver. The ops pointer check is simpler and cheaper, so I'd suggest that.
diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c index 073faa6a69b8..da72a990418d 100644 --- a/block/partitions/ibm.c +++ b/block/partitions/ibm.c @@ -23,6 +23,15 @@ union label_t { struct vtoc_cms_label cms; }; +struct dasd_gd_private { + void *devmap; + unsigned int cu_type; + unsigned int dev_type; + unsigned int label_block; + unsigned int format; + char type[4]; +}; + /* * compute the block number from a * cyl-cyl-head-head structure @@ -61,7 +70,7 @@ static sector_t cchhb2blk(struct vtoc_cchhb *ptr, struct hd_geometry *geo) } static int find_label(struct parsed_partitions *state, - dasd_information2_t *info, + struct dasd_gd_private *gd_priv, struct hd_geometry *geo, int blocksize, sector_t *labelsect, @@ -81,15 +90,16 @@ static int find_label(struct parsed_partitions *state, * - on an FBA disk it's block 1 * - on an CMS formatted FBA disk it is sector 1, even if the block size * is larger than 512 bytes (possible if the DIAG discipline is used) - * If we have a valid info structure, then we know exactly which case we - * have, otherwise we just search through all possebilities. + * If we have a valid dasd_gd_private structure, then we know exactly + * which case we have, otherwise we just search through all + * possibilities. */ - if (info) { - if ((info->cu_type == 0x6310 && info->dev_type == 0x9336) || - (info->cu_type == 0x3880 && info->dev_type == 0x3370)) - testsect[0] = info->label_block; + if (gd_priv) { + if ((gd_priv->cu_type == 0x6310 && gd_priv->dev_type == 0x9336) || + (gd_priv->cu_type == 0x3880 && gd_priv->dev_type == 0x3370)) + testsect[0] = gd_priv->label_block; else - testsect[0] = info->label_block * (blocksize >> 9); + testsect[0] = gd_priv->label_block * (blocksize >> 9); testcount = 1; } else { testsect[0] = 1; @@ -198,7 +208,7 @@ static int find_lnx1_partitions(struct parsed_partitions *state, union label_t *label, sector_t labelsect, loff_t i_size, - dasd_information2_t *info) + struct dasd_gd_private *gd_priv) { loff_t offset, geo_size, size; char tmp[64]; @@ -221,11 +231,11 @@ static int find_lnx1_partitions(struct parsed_partitions *state, * geo->sectors * secperblk; size = i_size >> 9; if (size != geo_size) { - if (!info) { + if (!gd_priv) { strlcat(state->pp_buf, "\n", PAGE_SIZE); return 1; } - if (!strcmp(info->type, "ECKD")) + if (!strcmp(gd_priv->type, "ECKD")) if (geo_size < size) size = geo_size; /* else keep size based on i_size */ @@ -289,9 +299,10 @@ static int find_cms1_partitions(struct parsed_partitions *state, int ibm_partition(struct parsed_partitions *state) { struct block_device *bdev = state->bdev; + struct dasd_gd_private *gd_priv = NULL; + struct gendisk *disk = bdev->bd_disk; int blocksize, res; loff_t i_size, offset, size; - dasd_information2_t *info; struct hd_geometry *geo; char type[5] = {0,}; char name[7] = {0,}; @@ -305,23 +316,21 @@ int ibm_partition(struct parsed_partitions *state) i_size = i_size_read(bdev->bd_inode); if (i_size == 0) goto out_exit; - info = kmalloc(sizeof(dasd_information2_t), GFP_KERNEL); - if (info == NULL) - goto out_exit; - geo = kmalloc(sizeof(struct hd_geometry), GFP_KERNEL); + geo = kzalloc(sizeof(struct hd_geometry), GFP_KERNEL); if (geo == NULL) - goto out_nogeo; + goto out_exit; label = kmalloc(sizeof(union label_t), GFP_KERNEL); if (label == NULL) goto out_nolab; - if (ioctl_by_bdev(bdev, HDIO_GETGEO, (unsigned long)geo) != 0) + geo->start = get_start_sect(bdev); + if (!disk->fops->getgeo || disk->fops->getgeo(bdev, geo)) goto out_freeall; - if (ioctl_by_bdev(bdev, BIODASDINFO2, (unsigned long)info) != 0) { - kfree(info); - info = NULL; - } - if (find_label(state, info, geo, blocksize, &labelsect, name, type, + /* gd_priv pointer is only valid for DASD devices */ + if (disk && disk->major == DASD_MAJOR) + gd_priv = disk->private_data; + + if (find_label(state, gd_priv, geo, blocksize, &labelsect, name, type, label)) { if (!strncmp(type, "VOL1", 4)) { res = find_vol1_partitions(state, geo, blocksize, name, @@ -329,24 +338,24 @@ int ibm_partition(struct parsed_partitions *state) } else if (!strncmp(type, "LNX1", 4)) { res = find_lnx1_partitions(state, geo, blocksize, name, label, labelsect, i_size, - info); + gd_priv); } else if (!strncmp(type, "CMS1", 4)) { res = find_cms1_partitions(state, geo, blocksize, name, label, labelsect); } - } else if (info) { + } else if (gd_priv) { /* * ugly but needed for backward compatibility: - * If the block device is a DASD (i.e. BIODASDINFO2 works), + * If the block device is a DASD (i.e. valid gd_priv), * then we claim it in any case, even though it has no valid * label. If it has the LDL format, then we simply define a * partition as if it had an LNX1 label. */ res = 1; - if (info->format == DASD_FORMAT_LDL) { + if (gd_priv->format == DASD_FORMAT_LDL) { strlcat(state->pp_buf, "(nonl)", PAGE_SIZE); size = i_size >> 9; - offset = (info->label_block + 1) * (blocksize >> 9); + offset = (gd_priv->label_block + 1) * (blocksize >> 9); put_partition(state, 1, offset, size-offset); strlcat(state->pp_buf, "\n", PAGE_SIZE); } @@ -357,8 +366,6 @@ int ibm_partition(struct parsed_partitions *state) kfree(label); out_nolab: kfree(geo); -out_nogeo: - kfree(info); out_exit: return res; } diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c index 32fc51341d99..63e48bf9cadc 100644 --- a/drivers/s390/block/dasd_devmap.c +++ b/drivers/s390/block/dasd_devmap.c @@ -677,18 +677,30 @@ dasd_device_from_cdev(struct ccw_device *cdev) void dasd_add_link_to_gendisk(struct gendisk *gdp, struct dasd_device *device) { + struct dasd_gd_private *gd_priv; struct dasd_devmap *devmap; + struct ccw_device *cdev; + gd_priv = kzalloc(sizeof(struct dasd_gd_private), GFP_KERNEL); + if (!gd_priv) + return; devmap = dasd_find_busid(dev_name(&device->cdev->dev)); if (IS_ERR(devmap)) return; + cdev = device->cdev; spin_lock(&dasd_devmap_lock); - gdp->private_data = devmap; + gd_priv->devmap = devmap; + gd_priv->cu_type = cdev->id.cu_type; + gd_priv->dev_type = cdev->id.dev_type; + memcpy(gd_priv->type, device->discipline->name, sizeof(gd_priv->type)); + device->discipline->fill_gd_priv(gd_priv, device); + gdp->private_data = gd_priv; spin_unlock(&dasd_devmap_lock); } struct dasd_device *dasd_device_from_gendisk(struct gendisk *gdp) { + struct dasd_gd_private *gd_priv; struct dasd_device *device; struct dasd_devmap *devmap; @@ -696,7 +708,8 @@ struct dasd_device *dasd_device_from_gendisk(struct gendisk *gdp) return NULL; device = NULL; spin_lock(&dasd_devmap_lock); - devmap = gdp->private_data; + gd_priv = gdp->private_data; + devmap = gd_priv->devmap; if (devmap && devmap->device) { device = devmap->device; dasd_get_device(device); diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c index facb588d09e4..38d20744df26 100644 --- a/drivers/s390/block/dasd_diag.c +++ b/drivers/s390/block/dasd_diag.c @@ -607,6 +607,15 @@ dasd_diag_fill_info(struct dasd_device * device, return 0; } +static void dasd_diag_fill_gd_priv(struct dasd_gd_private *gd_priv, + struct dasd_device *device) +{ + struct dasd_diag_private *private = device->private; + + gd_priv->label_block = (unsigned int) private->pt_block; + gd_priv->format = DASD_FORMAT_LDL; +} + static void dasd_diag_dump_sense(struct dasd_device *device, struct dasd_ccw_req * req, struct irb *stat) @@ -652,6 +661,7 @@ static struct dasd_discipline dasd_diag_discipline = { .free_cp = dasd_diag_free_cp, .dump_sense = dasd_diag_dump_sense, .fill_info = dasd_diag_fill_info, + .fill_gd_priv = dasd_diag_fill_gd_priv, }; static int __init diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c index ad44d22e8859..edaa7d1577a9 100644 --- a/drivers/s390/block/dasd_eckd.c +++ b/drivers/s390/block/dasd_eckd.c @@ -4901,6 +4901,15 @@ dasd_eckd_fill_info(struct dasd_device * device, return 0; } +static void dasd_eckd_fill_gd_priv(struct dasd_gd_private *gd_priv, + struct dasd_device *device) +{ + struct dasd_eckd_private *private = device->private; + + gd_priv->label_block = 2; + gd_priv->format = private->uses_cdl ? DASD_FORMAT_CDL : DASD_FORMAT_LDL; +} + /* * SECTION: ioctl functions for eckd devices. */ @@ -6727,6 +6736,7 @@ static struct dasd_discipline dasd_eckd_discipline = { .ext_pool_exhaust = dasd_eckd_ext_pool_exhaust, .ese_format = dasd_eckd_ese_format, .ese_read = dasd_eckd_ese_read, + .fill_gd_priv = dasd_eckd_fill_gd_priv, }; static int __init diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c index cbb770824226..6abbffdffb5f 100644 --- a/drivers/s390/block/dasd_fba.c +++ b/drivers/s390/block/dasd_fba.c @@ -642,6 +642,13 @@ dasd_fba_fill_info(struct dasd_device * device, return 0; } +static void dasd_fba_fill_gd_priv(struct dasd_gd_private *gd_priv, + struct dasd_device *device) +{ + gd_priv->label_block = 1; + gd_priv->format = DASD_FORMAT_LDL; +} + static void dasd_fba_dump_sense_dbf(struct dasd_device *device, struct irb *irb, char *reason) @@ -822,6 +829,7 @@ static struct dasd_discipline dasd_fba_discipline = { .dump_sense = dasd_fba_dump_sense, .dump_sense_dbf = dasd_fba_dump_sense_dbf, .fill_info = dasd_fba_fill_info, + .fill_gd_priv = dasd_fba_fill_gd_priv, }; static int __init diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index af5b0ecb8f89..d6ef85936526 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -87,6 +87,7 @@ void dasd_gendisk_free(struct dasd_block *block) { if (block->gdp) { del_gendisk(block->gdp); + kfree(block->gdp->private_data); block->gdp->private_data = NULL; put_disk(block->gdp); block->gdp = NULL; diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h index fa552f9f1666..31d12a62b28b 100644 --- a/drivers/s390/block/dasd_int.h +++ b/drivers/s390/block/dasd_int.h @@ -259,6 +259,15 @@ struct dasd_uid { char vduit[33]; }; +struct dasd_gd_private { + struct dasd_devmap *devmap; + unsigned int cu_type; + unsigned int dev_type; + unsigned int label_block; + unsigned int format; + char type[4]; +}; + /* * the struct dasd_discipline is * sth like a table of virtual functions, if you think of dasd_eckd @@ -391,6 +400,7 @@ struct dasd_discipline { struct dasd_ccw_req *(*ese_format)(struct dasd_device *, struct dasd_ccw_req *, struct irb *); int (*ese_read)(struct dasd_ccw_req *, struct irb *); + void (*fill_gd_priv)(struct dasd_gd_private *, struct dasd_device *); }; extern struct dasd_discipline *dasd_diag_discipline_pointer;