Hello,
This is an updated version of "dm/md dependency tree in sysfs" patch.
For example, if dm-0 maps to sda, we'll have following symlinks;
/sys/block/dm-0/slaves/sda --> /sys/block/sda
/sys/block/sda/holders/dm-0 --> /sys/block/dm-0
Difference from the previous version is a fix for a problem
of calling possibly-sleeping function inside the fast path.
There were 3 such calls in my previous bd_claim patch:
- kmalloc(GFP_KERNEL)
- sysfs_create_symlink()
- sysfs_remove_symlink()
The allocation code is moved outside of locks.
Symlink operations needs to be atomic, so they are moved out
of global bdev_lock and use bdev->bd_sem instead.
Comments are welcome.
I tested this on 2.6.16-rc6 and 2.6.16-rc6-mm1 with both
CONFIG_PREEMPT and CONFIG_DEBUG_SPINLOCK_SLEEP enabled
and saw no warnings in kernel log.
Patches included are:
1. [PATCH 1/7] kobject_add_dir
Adding kobject_add_dir() function which creates
a subdirectory for a given kobject.
2. [PATCH 2/7] add holders/slaves subdirectory to /sys/block
Creating "slaves" and "holders" directories in /sys/block/<disk>,
creating "holders" directory under /sys/block/<disk>/<partition>
3. [PATCH 3/7] bd_claim_by_kobject
Adding bd_claim_by_kobject() function which takes kobject as
additional signature of holder device and creates sysfs symlinks
between holder device and claimed device.
bd_release_from_kobject() is a counter part of bd_claim_by_kobject.
4. [PATCH 4/7] bd_claim_by_disk
Variants which take gendisk instead of kobject
and do kobject_{get,put}(&gendisk->kobj).
5. [PATCH 5/7] md to use bd_claim_by_disk
Use bd_claim_by_disk.
6. [PATCH 6/7] dm to use bd_claim_by_disk
Use bd_claim_by_disk.
7. [PATCH 7/7] conver bd_sem to bd_mutex
bd_sem is converted to bd_mutex in 2.6.16-rc6-mm1.
The patch follows that conversion.
Patch 3 includes the fix mentioned above.
Patch 6 depends on dm-table-store-md.patch and dm-tidy-mdptr.patch
which were committed to -mm yesterday.
Patch 7 depends on sem2mutex-blockdev-2.patch in 2.6.16-rc6-mm1.
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
With this patch, "slaves" and "holders" directories are
created in /sys/block/<disk> and
"holders" directory is created in /sys/block/<disk>/<partition>.
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
This adds kobject_add_dir() function which creates a subdirectory
for a given kobject.
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
This adds bd_claim_by_kobject() function which takes kobject as
additional signature of holder device and creates sysfs symlinks
between holder device and claimed device.
bd_release_from_kobject() is a counter part of bd_claim_by_kobject.
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
This adds variants of bd_claim_by_kobject which takes gendisk instead
of kobject and do kobject_{get,put}(&gendisk->slave_dir).
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
Following symlinks are created if md0 is built from sda and sdb
/sys/block/md0/slaves/sda --> /sys/block/sda
/sys/block/md0/slaves/sdb --> /sys/block/sdb
/sys/block/sda/holders/md0 --> /sys/block/md0
/sys/block/sdb/holders/md0 --> /sys/block/md0
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
Following symlinks are created if dm-0 maps to sda:
/sys/block/dm-0/slaves/sda --> /sys/block/sda
/sys/block/sda/holders/dm-0 --> /sys/block/dm-0
This patch depends on the following patches in mm tree:
dm-tidy-mdptr.patch
dm-table-store-md.patch
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
This patch is part of dm/md dependency tree in sysfs.
This patch follows the change introduced by
sem2mutex-blockdev-2.patch in 2.6.16-rc6-mm1.
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
"Jun'ichi Nomura" <[email protected]> wrote:
>
> This patch is part of dm/md dependency tree in sysfs.
>
> This adds bd_claim_by_kobject() function which takes kobject as
> additional signature of holder device and creates sysfs symlinks
> between holder device and claimed device.
> bd_release_from_kobject() is a counter part of bd_claim_by_kobject.
>
> ...
>
> @@ -402,6 +402,7 @@ struct block_device {
> struct list_head bd_inodes;
> void * bd_holder;
> int bd_holders;
> + struct list_head bd_holder_list;
> struct block_device * bd_contains;
> unsigned bd_block_size;
> struct hd_struct * bd_part;
Bear in mind that sysfs is Kconfigurable (CONFIG_SYSFS). If possible,
newly-added code which doesn't make sense without sysfs should not appear
in a CONFIG_SYSFS=n vmlinux.
> +
> +static inline struct kobject * bdev_get_kobj(struct block_device *bdev)
pet-peeve: The space after asterisk has no use.
> +{
> + if (bdev->bd_contains != bdev)
> + return kobject_get(&bdev->bd_part->kobj);
> + else
> + return kobject_get(&bdev->bd_disk->kobj);
> +}
> +
> +static inline struct kobject * bdev_get_holder(struct block_device *bdev)
> +static inline void add_symlink(struct kobject *from, struct kobject *to)
> +static inline void del_symlink(struct kobject *from, struct kobject *to)
> +static inline int bd_holder_grab_dirs(struct block_device *bdev,
> +static inline void bd_holder_release_dirs(struct bd_holder *bo)
I suposse the inlines are OK, given that we "know" that there's only a
single callsite. But recent gcc's take care of that automatically.
> +static int add_bd_holder(struct block_device *bdev, struct bd_holder *bo)
> +{
> + struct bd_holder *tmp;
> +
> + if (!bo)
> + return 0;
whitespace went wild there.
> +static void free_bd_holder(struct bd_holder *bo)
> ...
> +
> + bo->sdir = kobj;
> + list_for_each_entry(tmp, &bdev->bd_holder_list, list) {
> + if (tmp->sdir == bo->sdir) {
> + tmp->count++;
> + return 0;
> + }
> + }
> +
> + if (!bd_holder_grab_dirs(bdev, bo))
> + return 0;
> +
> + add_symlink(bo->sdir, bo->sdev);
> + add_symlink(bo->hdir, bo->hdev);
> + list_add_tail(&bo->list, &bdev->bd_holder_list);
> + return 1;
> +}
>
> +static struct bd_holder *del_bd_holder(struct block_device *bdev,
> + struct kobject *kobj)
> +{
> + struct bd_holder *bo;
> +
> + list_for_each_entry(bo, &bdev->bd_holder_list, list) {
> + if (bo->sdir == kobj) {
> + bo->count--;
> + BUG_ON(bo->count < 0);
> + if (!bo->count) {
> + list_del(&bo->list);
> + del_symlink(bo->sdir, bo->sdev);
> + del_symlink(bo->hdir, bo->hdev);
> + bd_holder_release_dirs(bo);
> + return bo;
> + }
> + break;
> + }
> + }
> +
> + return NULL;
> +}
Some comments which describe what these do, what they return and why they
return it would be nice.
> +
> +int bd_claim_by_kobject(struct block_device *bdev, void *holder,
> + struct kobject *kobj)
> +{
Ditto.
> + int res = -EBUSY;
> + struct bd_holder *bo;
> +
> + if (!kobj)
> + return -EINVAL;
> +
> + bo = alloc_bd_holder(kobj);
> + if (!bo)
> + return -ENOMEM;
> +
> + down(&bdev->bd_sem);
> + res = bd_claim(bdev, holder);
> + if (res || !add_bd_holder(bdev, bo))
> + free_bd_holder(bo);
> + up(&bdev->bd_sem);
> +
> + return res;
> +}
> +
> +EXPORT_SYMBOL(bd_claim_by_kobject);
I don't think the blank line before the EXPORT_SYMBOL() does anything useful.
EXPORT_SYMBOL_GPL() might be preferred.
> +void bd_release_from_kobject(struct block_device *bdev, struct kobject *kobj)
> +{
> + struct bd_holder *bo;
> +
> + if (!kobj)
> + return;
> +
> + down(&bdev->bd_sem);
> + bd_release(bdev);
> + if ((bo = del_bd_holder(bdev, kobj)))
> + free_bd_holder(bo);
> + up(&bdev->bd_sem);
> +}
> +
> +EXPORT_SYMBOL(bd_release_from_kobject);
Ditto.
"Jun'ichi Nomura" <[email protected]> wrote:
>
> Variants of bd_claim_by_kobject which takes gendisk instead
> of kobject and do kobject_{get,put}(&gendisk->slave_dir).
>
> Signed-off-by: Jun'ichi Nomura <[email protected]>
>
> include/linux/genhd.h | 13 +++++++++++++
> 1 files changed, 13 insertions(+)
>
> --- linux-2.6.16-rc6.orig/include/linux/genhd.h 2006-03-11 17:12:55.000000000 -0500
> +++ linux-2.6.16-rc6/include/linux/genhd.h 2006-03-13 11:24:13.000000000 -0500
> @@ -421,6 +424,19 @@ static inline struct block_device *bdget
> return bdget(MKDEV(disk->major, disk->first_minor) + index);
> }
>
> +static inline int bd_claim_by_disk(struct block_device *bdev,
> + void *holder, struct gendisk *disk)
> +{
> + return bd_claim_by_kobject(bdev, holder, kobject_get(disk->slave_dir));
> +}
> +
> +static inline void bd_release_from_disk(struct block_device *bdev,
> + struct gendisk *disk)
> +{
> + bd_release_from_kobject(bdev, disk->slave_dir);
> + kobject_put(disk->slave_dir);
> +}
> +
genhd.h doesn't include kobject.h, so this only compiles due to luck.
An alternative to adding possibly risky nested header inclusions would be
to uninline these functions.
Hi Andrew,
Thank you for the review and comments.
Attached patch reflects your comments.
The patch also reflects the other comment regarding uninlining
bd_claim_by_disk() and bd_release_from_disk().
Could you please replace
dm-md-dependency-tree-in-sysfs-bd_claim_by_kobject.patch with this
and drop dm-md-dependency-tree-in-sysfs-bd_claim_by_disk.patch?
Or am I better to send a patch relative to the previous ones?
Andrew Morton wrote:
> Bear in mind that sysfs is Kconfigurable (CONFIG_SYSFS). If possible,
> newly-added code which doesn't make sense without sysfs should not appear
> in a CONFIG_SYSFS=n vmlinux.
OK, fixed.
CONFIG_SYSFS=n built ok and confirmed no symbols related to
my sysfs symlinking exist in it.
>>+static inline struct kobject * bdev_get_kobj(struct block_device *bdev)
>
> pet-peeve: The space after asterisk has no use.
Fixed.
>>+static inline struct kobject * bdev_get_holder(struct block_device *bdev)
>>+static inline void add_symlink(struct kobject *from, struct kobject *to)
>>+static inline void del_symlink(struct kobject *from, struct kobject *to)
>>+static inline int bd_holder_grab_dirs(struct block_device *bdev,
>>+static inline void bd_holder_release_dirs(struct bd_holder *bo)
>
> I suposse the inlines are OK, given that we "know" that there's only a
> single callsite. But recent gcc's take care of that automatically.
I removed 'inline's from them and let it cared by gcc.
>>+static int add_bd_holder(struct block_device *bdev, struct bd_holder *bo)
>>+{
>>+ struct bd_holder *tmp;
>>+
>>+ if (!bo)
>>+ return 0;
>
> whitespace went wild there.
Fixed. Thank you.
> Some comments which describe what these do, what they return and why they
> return it would be nice.
Comments are added for non-trivial functions and a structure.
>>+}
>>+
>>+EXPORT_SYMBOL(bd_claim_by_kobject);
>
> I don't think the blank line before the EXPORT_SYMBOL() does anything useful.
Sure. They just followed the other exports in the file.
Fixed.
> EXPORT_SYMBOL_GPL() might be preferred.
I changed them to EXPORT_SYMBOL_GPL().
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.
Hi Andrew,
Attached patch makes sysfs symlinking related codes conditional
on CONFIG_SYSFS.
Could you please replace
dm-md-dependency-tree-in-sysfs-holders-slaves-subdirectory.patch
with this?
Jun'ichi Nomura wrote:
> This patch is part of dm/md dependency tree in sysfs.
>
> With this patch, "slaves" and "holders" directories are
> created in /sys/block/<disk> and
> "holders" directory is created in /sys/block/<disk>/<partition>.
Thanks,
--
Jun'ichi Nomura, NEC Solutions (America), Inc.