ACK: [SRU][Xenial][Artful][Bionic][v2][PATCH 1/1] UBUNTU: SAUCE: (no-up) bcache: decouple emitting a cached_dev CHANGE uevent

[Joseph Salisbury]

On 01/23/2018 11:27 AM, Seth Forshee wrote:
> On Tue, Jan 23, 2018 at 02:33:31PM +0000, Colin Ian King wrote:
>> On 11/12/17 14:12, Joseph Salisbury wrote:
>>> From: Ryan Harper <ryan.harper at canonical.com>
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/1729145
>>>
>>> - decouple emitting a cached_dev CHANGE uevent which includes dev.uuid
>>>   and dev.label from bch_cached_dev_run() which only happens when a
>>>   bcacheX device is bound to the actual backing block device (bcache0 -> vdb)
>>>
>>> - update bch_cached_dev_run() to invoke bch_cached_dev_emit_change() as
>>>   needed; no functional code path changes here
>>>
>>> - Modify register_bcache to detect a re-registering of a bcache
>>>   cached_dev, and in that case call bcache_cached_dev_emit_change() to
>>>
>>> Signed-off-by: Ryan Harper <ryan.harper at canonical.com>
>>> Signed-off-by: Joseph Salisbury <joseph.salisbury at canonical.com>
>>> ---
>>>  drivers/md/bcache/bcache.h |  1 +
>>>  drivers/md/bcache/super.c  | 53 +++++++++++++++++++++++++++++++++++++---------
>>>  2 files changed, 44 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
>>> index 02619ca..8ed7e7a 100644
>>> --- a/drivers/md/bcache/bcache.h
>>> +++ b/drivers/md/bcache/bcache.h
>>> @@ -906,6 +906,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size);
>>>  
>>>  int bch_cached_dev_attach(struct cached_dev *, struct cache_set *);
>>>  void bch_cached_dev_detach(struct cached_dev *);
>>> +void bch_cached_dev_emit_change(struct cached_dev *);
>>>  void bch_cached_dev_run(struct cached_dev *);
>>>  void bcache_device_stop(struct bcache_device *);
>>>  
>>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>>> index 04608da..9ff0aac 100644
>>> --- a/drivers/md/bcache/super.c
>>> +++ b/drivers/md/bcache/super.c
>>> @@ -834,7 +834,7 @@ static void calc_cached_dev_sectors(struct cache_set *c)
>>>  	c->cached_dev_sectors = sectors;
>>>  }
>>>  
>>> -void bch_cached_dev_run(struct cached_dev *dc)
>>> +void bch_cached_dev_emit_change(struct cached_dev *dc)
>>>  {
>>>  	struct bcache_device *d = &dc->disk;
>>>  	char buf[SB_LABEL_SIZE + 1];
>>> @@ -849,9 +849,18 @@ void bch_cached_dev_run(struct cached_dev *dc)
>>>  	buf[SB_LABEL_SIZE] = '\0';
>>>  	env[2] = kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf);
>>>  
>>> +	/* won't show up in the uevent file, use udevadm monitor -e instead
>>> +	 * only class / kset properties are persistent */
>>> +	kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
>>> +	kfree(env[1]);
>>> +	kfree(env[2]);
>>> +
>>> +}
>>> +
>>> +void bch_cached_dev_run(struct cached_dev *dc)
>>> +{
>>> +	struct bcache_device *d = &dc->disk;
>>>  	if (atomic_xchg(&dc->running, 1)) {
>>> -		kfree(env[1]);
>>> -		kfree(env[2]);
>>>  		return;
>>>  	}
>>>  
>>> @@ -867,11 +876,9 @@ void bch_cached_dev_run(struct cached_dev *dc)
>>>  
>>>  	add_disk(d->disk);
>>>  	bd_link_disk_holder(dc->bdev, dc->disk.disk);
>>> -	/* won't show up in the uevent file, use udevadm monitor -e instead
>>> -	 * only class / kset properties are persistent */
>>> -	kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
>>> -	kfree(env[1]);
>>> -	kfree(env[2]);
>>> +
>>> +	/* emit change event */
>>> +	bch_cached_dev_emit_change(dc);
>>>  
>>>  	if (sysfs_create_link(&d->kobj, &disk_to_dev(d->disk)->kobj, "dev") ||
>>>  	    sysfs_create_link(&disk_to_dev(d->disk)->kobj, &d->kobj, "bcache"))
>>> @@ -1916,6 +1923,21 @@ static bool bch_is_open_backing(struct block_device *bdev) {
>>>  	return false;
>>>  }
>>>  
>>> +static struct cached_dev *bch_find_cached_dev(struct block_device *bdev) {
>>> +	struct cache_set *c, *tc;
>>> +	struct cached_dev *dc, *t;
>>> +
>>> +	list_for_each_entry_safe(c, tc, &bch_cache_sets, list)
>>> +		list_for_each_entry_safe(dc, t, &c->cached_devs, list)
>>> +			if (dc->bdev == bdev)
>>> +				return dc;
>>> +	list_for_each_entry_safe(dc, t, &uncached_devices, list)
>>> +		if (dc->bdev == bdev)
>>> +			return dc;
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>>  static bool bch_is_open_cache(struct block_device *bdev) {
>>>  	struct cache_set *c, *tc;
>>>  	struct cache *ca;
>>> @@ -1941,6 +1963,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>>>  	struct cache_sb *sb = NULL;
>>>  	struct block_device *bdev = NULL;
>>>  	struct page *sb_page = NULL;
>>> +	struct cached_dev *dc = NULL;
>>>  
>>>  	if (!try_module_get(THIS_MODULE))
>>>  		return -EBUSY;
>>> @@ -1957,10 +1980,20 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>>>  		if (bdev == ERR_PTR(-EBUSY)) {
>>>  			bdev = lookup_bdev(strim(path), 0);
>>>  			mutex_lock(&bch_register_lock);
>>> -			if (!IS_ERR(bdev) && bch_is_open(bdev))
>>> +			if (!IS_ERR(bdev) && bch_is_open(bdev)) {
>>>  				err = "device already registered";
>>> -			else
>>> +				/* emit CHANGE event for backing devices to export
>>> +				 * CACHED_{UUID/LABEL} values to udev */
>>> +				if (bch_is_open_backing(bdev)) {
>>> +					dc = bch_find_cached_dev(bdev);
>>> +					if (dc) {
>>> +						bch_cached_dev_emit_change(dc);
>>> +						err = "device already registered (emitting change event)";
>>> +					}
>>> +				}
>>> +			} else {
>>>  				err = "device busy";
>>> +            }
>>>  			mutex_unlock(&bch_register_lock);
>>>  			if (!IS_ERR(bdev))
>>>  				bdput(bdev);
>>>
>> Positive test results for Bionic. Looks reasonable to me.
>>
>> Will this be sent upstream at some point?
> +1 to this question. This is a userspace-visible change in behavior, so
> I'm somewhat hesitant to apply it if the behavior won't also change
> upstream.
>
I'll submit this upstream for Ryan.