Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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.
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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.
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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.
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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.
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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?
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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?
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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?
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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.
Re: [PATCH 1/1] s390/dasd: remove ioctl_by_bdev from DASD driver
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.
