Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1434177 > unrolled thread
| Started by | Fam Zheng <famz@redhat.com> |
|---|---|
| First post | 2016-06-30 04:10 +0200 |
| Last post | 2016-07-01 03:30 +0200 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v2 00/12] gendisk: Generate uevent after attribute available Fam Zheng <famz@redhat.com> - 2016-06-30 04:10 +0200
Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available Christoph Hellwig <hch@infradead.org> - 2016-06-30 08:30 +0200
Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available Fam Zheng <famz@redhat.com> - 2016-06-30 08:40 +0200
Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available Christoph Hellwig <hch@infradead.org> - 2016-06-30 08:50 +0200
Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available Fam Zheng <famz@redhat.com> - 2016-07-01 03:10 +0200
Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available Dan Williams <dan.j.williams@intel.com> - 2016-07-01 03:30 +0200
| From | Fam Zheng <famz@redhat.com> |
|---|---|
| Date | 2016-06-30 04:10 +0200 |
| Subject | [PATCH v2 00/12] gendisk: Generate uevent after attribute available |
| Message-ID | <rPzAt-3BV-5@gated-at.bofh.it> |
The race condition is noticed between disk_add() and disk attributes, on virtio-blk hotplug. Userspace listens to the KOBJ_ADD uevent generated in add_disk(). At that point we haven't created the serial attribute file, therefore depending on how fast udev reacts, the /dev/disk/by-id/ entry doesn't always get created. As pointed out by Christoph Hellwig in the specific fix [1], virtio-blk is not the only driver that suffers from this, so we cannot count on every driver to send events manually. Moreover as suggested in uevent documentation, it is advised to defer the KOBJ_ADD event until all attributes are ready: Documentation/kobject.txt: > Use the KOBJ_ADD action for when the kobject is first added to the kernel. > This should be done only after any attributes or children of the kobject > have been initialized properly, as userspace will instantly start to look > for them when this call happens. Unfortunately it seems impossible to fix this generally without touching the offending callers. The approach I'm proposing here is adding a flag to suppress uevent in add_disk(), which is patch 1, then in later patches, convert any caller to only trigger the uevent when attributes are added. [1] https://lkml.org/lkml/2016/6/28/550 Fam Zheng (12): genhd: Add "gen_uevent" parameter to add_disk genhd: Honor gen_uevent and add disk_gen_uevents virtio-blk: Generate uevent after attribute available axonrom: Generate uevent after attribute available aoeblk: Generate uevent after attribute available mtip32xx: Generate uevent after attribute available pktcdvd: Generate uevent after attribute available zram: Generate uevent after attribute available md: Generate uevent after attribute available mmc: Generate uevent after attribute available mtd: Generate uevent after attribute available nvme: Generate uevent after attribute available arch/m68k/emu/nfblock.c | 2 +- arch/powerpc/sysdev/axonram.c | 3 ++- arch/um/drivers/ubd_kern.c | 2 +- arch/xtensa/platforms/iss/simdisk.c | 2 +- block/genhd.c | 26 +++++++++++++++++++++----- drivers/block/DAC960.c | 2 +- drivers/block/amiflop.c | 2 +- drivers/block/aoe/aoeblk.c | 3 ++- drivers/block/ataflop.c | 2 +- drivers/block/brd.c | 4 ++-- drivers/block/cciss.c | 2 +- drivers/block/drbd/drbd_main.c | 2 +- drivers/block/floppy.c | 2 +- drivers/block/hd.c | 2 +- drivers/block/loop.c | 2 +- drivers/block/mg_disk.c | 2 +- drivers/block/mtip32xx/mtip32xx.c | 3 ++- drivers/block/nbd.c | 2 +- drivers/block/null_blk.c | 2 +- drivers/block/osdblk.c | 2 +- drivers/block/paride/pcd.c | 2 +- drivers/block/paride/pd.c | 2 +- drivers/block/paride/pf.c | 2 +- drivers/block/pktcdvd.c | 4 +++- drivers/block/ps3disk.c | 2 +- drivers/block/ps3vram.c | 2 +- drivers/block/rbd.c | 2 +- drivers/block/rsxx/dev.c | 2 +- drivers/block/skd_main.c | 2 +- drivers/block/sunvdc.c | 2 +- drivers/block/swim.c | 2 +- drivers/block/swim3.c | 2 +- drivers/block/sx8.c | 2 +- drivers/block/umem.c | 2 +- drivers/block/virtio_blk.c | 3 ++- drivers/block/xen-blkfront.c | 2 +- drivers/block/xsysace.c | 2 +- drivers/block/z2ram.c | 2 +- drivers/block/zram/zram_drv.c | 3 ++- drivers/cdrom/gdrom.c | 2 +- drivers/ide/ide-cd.c | 2 +- drivers/ide/ide-gd.c | 2 +- drivers/lightnvm/core.c | 2 +- drivers/md/bcache/super.c | 4 ++-- drivers/md/dm.c | 2 +- drivers/md/md.c | 3 ++- drivers/memstick/core/ms_block.c | 2 +- drivers/memstick/core/mspro_block.c | 2 +- drivers/mmc/card/block.c | 3 ++- drivers/mtd/mtd_blkdevs.c | 3 ++- drivers/mtd/ubi/block.c | 2 +- drivers/nvdimm/blk.c | 2 +- drivers/nvdimm/btt.c | 2 +- drivers/nvdimm/pmem.c | 2 +- drivers/nvme/host/core.c | 3 ++- drivers/s390/block/dasd_genhd.c | 2 +- drivers/s390/block/dcssblk.c | 2 +- drivers/s390/block/scm_blk.c | 2 +- drivers/s390/block/xpram.c | 2 +- drivers/sbus/char/jsflash.c | 2 +- drivers/scsi/sd.c | 2 +- drivers/scsi/sr.c | 2 +- drivers/staging/lustre/lustre/llite/lloop.c | 2 +- include/linux/genhd.h | 3 ++- 64 files changed, 98 insertions(+), 70 deletions(-) -- 2.9.0
[toc] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2016-06-30 08:30 +0200 |
| Subject | Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available |
| Message-ID | <rPDE6-67b-35@gated-at.bofh.it> |
| In reply to | #1434177 |
On Thu, Jun 30, 2016 at 09:59:41AM +0800, Fam Zheng wrote: > Documentation/kobject.txt: > > Use the KOBJ_ADD action for when the kobject is first added to the kernel. > > This should be done only after any attributes or children of the kobject > > have been initialized properly, as userspace will instantly start to look > > for them when this call happens. > > Unfortunately it seems impossible to fix this generally without touching the > offending callers. The approach I'm proposing here is adding a flag to > suppress uevent in add_disk(), which is patch 1, then in later patches, convert > any caller to only trigger the uevent when attributes are added. We (or rather Dan) is touching most add_disk callers anyway for the driverfs_dev removal. Let's just pass the array of attributes to a disk_add variant and solve the issue for real.
[toc] | [prev] | [next] | [standalone]
| From | Fam Zheng <famz@redhat.com> |
|---|---|
| Date | 2016-06-30 08:40 +0200 |
| Subject | Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available |
| Message-ID | <rPDNM-6av-11@gated-at.bofh.it> |
| In reply to | #1434244 |
On Wed, 06/29 23:24, Christoph Hellwig wrote: > On Thu, Jun 30, 2016 at 09:59:41AM +0800, Fam Zheng wrote: > > Documentation/kobject.txt: > > > Use the KOBJ_ADD action for when the kobject is first added to the kernel. > > > This should be done only after any attributes or children of the kobject > > > have been initialized properly, as userspace will instantly start to look > > > for them when this call happens. > > > > Unfortunately it seems impossible to fix this generally without touching the > > offending callers. The approach I'm proposing here is adding a flag to > > suppress uevent in add_disk(), which is patch 1, then in later patches, convert > > any caller to only trigger the uevent when attributes are added. > > We (or rather Dan) is touching most add_disk callers anyway for the > driverfs_dev removal. Let's just pass the array of attributes to > a disk_add variant and solve the issue for real. I thought about that. Its usage is more compact compared to this series, but is also more code and less flexible IMO. For example, we need at least two variants, for attribute_group and device_attribute separately, right? Fam
[toc] | [prev] | [next] | [standalone]
| From | Christoph Hellwig <hch@infradead.org> |
|---|---|
| Date | 2016-06-30 08:50 +0200 |
| Subject | Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available |
| Message-ID | <rPDXs-6dZ-31@gated-at.bofh.it> |
| In reply to | #1434249 |
On Thu, Jun 30, 2016 at 02:35:54PM +0800, Fam Zheng wrote: > also more code and less flexible IMO. For example, we need at least two > variants, for attribute_group and device_attribute separately, right? Yes, or maybe just a calling convention that just passes both.
[toc] | [prev] | [next] | [standalone]
| From | Fam Zheng <famz@redhat.com> |
|---|---|
| Date | 2016-07-01 03:10 +0200 |
| Subject | Re: [PATCH v2 00/12] gendisk: Generate uevent after attribute available |
| Message-ID | <rPV7Y-8ql-7@gated-at.bofh.it> |
| In reply to | #1434257 |
On Wed, 06/29 23:38, Christoph Hellwig wrote: > On Thu, Jun 30, 2016 at 02:35:54PM +0800, Fam Zheng wrote: > > also more code and less flexible IMO. For example, we need at least two > > variants, for attribute_group and device_attribute separately, right? > > Yes, or maybe just a calling convention that just passes both. OK, I can look into that, but I'm not sure about the error handling. Currently add_disk returns void, do you have any plan on that too? should I change it in v3 (to at least return the attribute creation failure)? Fam
[toc] | [prev] | [next] | [standalone]
| From | Dan Williams <dan.j.williams@intel.com> |
|---|---|
| Date | 2016-07-01 03:30 +0200 |
| Message-ID | <rPVrj-71-17@gated-at.bofh.it> |
| In reply to | #1434939 |
On Thu, Jun 30, 2016 at 6:01 PM, Fam Zheng <famz@redhat.com> wrote:
> On Wed, 06/29 23:38, Christoph Hellwig wrote:
>> On Thu, Jun 30, 2016 at 02:35:54PM +0800, Fam Zheng wrote:
>> > also more code and less flexible IMO. For example, we need at least two
>> > variants, for attribute_group and device_attribute separately, right?
>>
>> Yes, or maybe just a calling convention that just passes both.
>
> OK, I can look into that, but I'm not sure about the error handling. Currently
> add_disk returns void, do you have any plan on that too? should I change it in
> v3 (to at least return the attribute creation failure)?
I think we should only support a "groups" interface to
device_add_disk() and convert all the drivers that currently do
device_create_file() after add_disk() to pass in a group list instead.
That way we follow the expectation that the only way to get an
attribute for a device to show up before KOBJ_ADD, is to define a
group:
From Documentation/driver-model/device.txt:
As explained in Documentation/kobject.txt, device attributes must be be
created before the KOBJ_ADD uevent is generated. The only way to realize
that is by defining an attribute group.
Let's defer the return value fixing for now.
You can find the pending device_add_disk() patches in the nvdimm
patchwork starting with "block: introduce device_add_disk()"
https://patchwork.kernel.org/project/linux-nvdimm/list/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web