Hi Jens and DASD maintainers,
can you take a look at this series, which stops the DASD driver from
issuing ioctls from kernel space, in preparation of removing
ioctl_by_bdev. I don't really like the new s390-only method, but short
of forcing the dasd driver to be built into the kernel I can't think of
anything better. But maybe the s390 maintainers are fine with forcing
the DASD driver to be built in, in which case we could go down that
route?
Just call the getgeo and biodasdinfo methods directly.
Signed-off-by: Christoph Hellwig <[email protected]>
---
block/partitions/ibm.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c
index 073faa6a69b8..21dc6da20ff2 100644
--- a/block/partitions/ibm.c
+++ b/block/partitions/ibm.c
@@ -289,6 +289,7 @@ static int find_cms1_partitions(struct parsed_partitions *state,
int ibm_partition(struct parsed_partitions *state)
{
struct block_device *bdev = state->bdev;
+ struct gendisk *disk = bdev->bd_disk;
int blocksize, res;
loff_t i_size, offset, size;
dasd_information2_t *info;
@@ -308,15 +309,16 @@ int ibm_partition(struct parsed_partitions *state)
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;
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) {
+ if (!disk->fops->biodasdinfo || disk->fops->biodasdinfo(disk, info)) {
kfree(info);
info = NULL;
}
--
2.26.1
Prepare for in-kernel callers of this functionality.
Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/s390/block/dasd_ioctl.c | 38 +++++++++++++++++++--------------
1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index 9a5f3add325f..dabcb4ce92da 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -457,10 +457,9 @@ static int dasd_ioctl_read_profile(struct dasd_block *block, void __user *argp)
/*
* Return dasd information. Used for BIODASDINFO and BIODASDINFO2.
*/
-static int dasd_ioctl_information(struct dasd_block *block,
- unsigned int cmd, void __user *argp)
+static int __dasd_ioctl_information(struct dasd_block *block,
+ struct dasd_information2_t *dasd_info)
{
- struct dasd_information2_t *dasd_info;
struct subchannel_id sch_id;
struct ccw_dev_id dev_id;
struct dasd_device *base;
@@ -473,10 +472,6 @@ static int dasd_ioctl_information(struct dasd_block *block,
if (!base->discipline || !base->discipline->fill_info)
return -EINVAL;
- dasd_info = kzalloc(sizeof(struct dasd_information2_t), GFP_KERNEL);
- if (dasd_info == NULL)
- return -ENOMEM;
-
rc = base->discipline->fill_info(base, dasd_info);
if (rc) {
kfree(dasd_info);
@@ -520,15 +515,24 @@ static int dasd_ioctl_information(struct dasd_block *block,
list_for_each(l, &base->ccw_queue)
dasd_info->chanq_len++;
spin_unlock_irqrestore(&block->queue_lock, flags);
+ return 0;
+}
- rc = 0;
- if (copy_to_user(argp, dasd_info,
- ((cmd == (unsigned int) BIODASDINFO2) ?
- sizeof(struct dasd_information2_t) :
- sizeof(struct dasd_information_t))))
- rc = -EFAULT;
+static int dasd_ioctl_information(struct dasd_block *block, void __user *argp,
+ size_t copy_size)
+{
+ struct dasd_information2_t *dasd_info;
+ int error = 0;
+
+ dasd_info = kzalloc(sizeof(*dasd_info), GFP_KERNEL);
+ if (!dasd_info)
+ return -ENOMEM;
+
+ error = __dasd_ioctl_information(block, dasd_info);
+ if (!error && copy_to_user(argp, dasd_info, copy_size))
+ error = -EFAULT;
kfree(dasd_info);
- return rc;
+ return error;
}
/*
@@ -622,10 +626,12 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
rc = dasd_ioctl_check_format(bdev, argp);
break;
case BIODASDINFO:
- rc = dasd_ioctl_information(block, cmd, argp);
+ rc = dasd_ioctl_information(block, argp,
+ sizeof(struct dasd_information_t));
break;
case BIODASDINFO2:
- rc = dasd_ioctl_information(block, cmd, argp);
+ rc = dasd_ioctl_information(block, argp,
+ sizeof(struct dasd_information2_t));
break;
case BIODASDPRRD:
rc = dasd_ioctl_read_profile(block, argp);
--
2.26.1
On 21.04.20 08:12, Christoph Hellwig wrote:
> Hi Jens and DASD maintainers,
>
> can you take a look at this series, which stops the DASD driver from
> issuing ioctls from kernel space, in preparation of removing
> ioctl_by_bdev. I don't really like the new s390-only method, but short
> of forcing the dasd driver to be built into the kernel I can't think of
> anything better. But maybe the s390 maintainers are fine with forcing
> the DASD driver to be built in, in which case we could go down that
> route?
Hmm the defconfig results in dasd built-in anyway. But distros really like
to keep it modular.
Hmm, we do have
obj-$(CONFIG_DASD) += dasd_mod.o
obj-$(CONFIG_DASD_DIAG) += dasd_diag_mod.o
obj-$(CONFIG_DASD_ECKD) += dasd_eckd_mod.o
obj-$(CONFIG_DASD_FBA) += dasd_fba_mod.o
Would it work to make CONFIG_DASD built-in only and keep the other 3 as modules?
Not sure about the implications.
On Tue, 21 Apr 2020 11:58:31 +0200
Christian Borntraeger <[email protected]> wrote:
> On 21.04.20 08:12, Christoph Hellwig wrote:
> > Hi Jens and DASD maintainers,
> >
> > can you take a look at this series, which stops the DASD driver from
> > issuing ioctls from kernel space, in preparation of removing
> > ioctl_by_bdev. I don't really like the new s390-only method, but short
> > of forcing the dasd driver to be built into the kernel I can't think of
> > anything better. But maybe the s390 maintainers are fine with forcing
> > the DASD driver to be built in, in which case we could go down that
> > route?
>
> Hmm the defconfig results in dasd built-in anyway. But distros really like
> to keep it modular.
>
> Hmm, we do have
>
> obj-$(CONFIG_DASD) += dasd_mod.o
> obj-$(CONFIG_DASD_DIAG) += dasd_diag_mod.o
> obj-$(CONFIG_DASD_ECKD) += dasd_eckd_mod.o
> obj-$(CONFIG_DASD_FBA) += dasd_fba_mod.o
>
> Would it work to make CONFIG_DASD built-in only and keep the other 3 as modules?
> Not sure about the implications.
>
I don't think non-eckd dasd drivers are really useful outside of z/VM
guests, so keeping at least the disciplines modular would be good.
Also, what about special purpose environments like the zfcp dumper?
Would be good to be able to keep these small.
How big is the dasd code in the end?
On 21.04.20 12:32, Cornelia Huck wrote:
> On Tue, 21 Apr 2020 11:58:31 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 21.04.20 08:12, Christoph Hellwig wrote:
>>> Hi Jens and DASD maintainers,
>>>
>>> can you take a look at this series, which stops the DASD driver from
>>> issuing ioctls from kernel space, in preparation of removing
>>> ioctl_by_bdev. I don't really like the new s390-only method, but short
>>> of forcing the dasd driver to be built into the kernel I can't think of
>>> anything better. But maybe the s390 maintainers are fine with forcing
>>> the DASD driver to be built in, in which case we could go down that
>>> route?
>>
>> Hmm the defconfig results in dasd built-in anyway. But distros really like
>> to keep it modular.
>>
>> Hmm, we do have
>>
>> obj-$(CONFIG_DASD) += dasd_mod.o
>> obj-$(CONFIG_DASD_DIAG) += dasd_diag_mod.o
>> obj-$(CONFIG_DASD_ECKD) += dasd_eckd_mod.o
>> obj-$(CONFIG_DASD_FBA) += dasd_fba_mod.o
>>
>> Would it work to make CONFIG_DASD built-in only and keep the other 3 as modules?
>> Not sure about the implications.
>>
>
> I don't think non-eckd dasd drivers are really useful outside of z/VM
> guests, so keeping at least the disciplines modular would be good.
>
> Also, what about special purpose environments like the zfcp dumper?
> Would be good to be able to keep these small.
>
> How big is the dasd code in the end?
File: drivers/s390/block/dasd_diag_mod.ko
Size: 519976 Blocks: 1016 IO Block: 4096 regular file
--
File: drivers/s390/block/dasd_eckd_mod.ko
Size: 2125976 Blocks: 4160 IO Block: 4096 regular file
--
File: drivers/s390/block/dasd_fba_mod.ko
Size: 524256 Blocks: 1024 IO Block: 4096 regular file
--
File: drivers/s390/block/dasd_mod.ko
Size: 3273464 Blocks: 6400 IO Block: 4096 regular file
So 3 MB seems quite a lot for special purpose Linuxes like the zfcp dumper.
On Tue, Apr 21, 2020 at 12:43:03PM +0200, Christian Borntraeger wrote:
> So 3 MB seems quite a lot for special purpose Linuxes like the zfcp dumper.
Does the zfcp dumper need DASD support at all? We don't have to always
build in the DASD core to avoid the strange dasd-specific block_device
operation, but only ensure it is built-in if it is selected at all.
Hi Christoph,
thanks for addressing this. But I must say that I do not like this approach.
I get your point why you want this ioctl to be removed and I agree with
that.
Having those implicit partitions at all is really ugly but I fear that this
is widely used int the field. So I can not simply remove this code although
I would like to. Maybe we find a way to deprecate this.
But anyway...
Forcing the driver to be build in may have a lot of implications which we
should at least have a look at and maybe discuss with the distributors.
All major distributions have the driver build as modules and use module
parameters in the initrd for example.
The second thing is that I do not really like this s390-specific blockdevice
operation.
I can imagine some ways to get rid of this ioctl_by_bdev. Maybe having a
udev
rule to add a partition from userspace or having the driver add the implicit
partition at the end. Or maybe something else.
If it is OK I will have a look at this and discuss this issue with my
colleagues and come up with a different approach.
Regards,
Stefan
Am 21.04.20 um 08:12 schrieb Christoph Hellwig:
> Hi Jens and DASD maintainers,
>
> can you take a look at this series, which stops the DASD driver from
> issuing ioctls from kernel space, in preparation of removing
> ioctl_by_bdev. I don't really like the new s390-only method, but short
> of forcing the dasd driver to be built into the kernel I can't think of
> anything better. But maybe the s390 maintainers are fine with forcing
> the DASD driver to be built in, in which case we could go down that
> route?
On Tue, Apr 21, 2020 at 04:17:53PM +0200, Stefan Haberland wrote:
> I can imagine some ways to get rid of this ioctl_by_bdev. Maybe having a
> udev
> rule to add a partition from userspace or having the driver add the implicit
> partition at the end. Or maybe something else.
>
> If it is OK I will have a look at this and discuss this issue with my
> colleagues and come up with a different approach.
Sure, we can wait a few days. Note that I don't want to break existing
userspace, which kinda speaks against a udev solution.