A few more md patches for 2.6.26
cover A few more md patches for 2.6.26
Hi Neil,
The first two patches add some necessary sysfs_notify calls for external metadata arrays. They replace md-ping-userspace-on-ext-metadata-events.patch which, as you noted, did these notifications globally in md_check_recovery() rather than at the local site where the state changes.
While testing these I ran into:
WARNING: at include/linux/blkdev.h:443 queue_flag_clear+0x2c/0x4d()
...due to the recent "block: make queue flags non-atomic" (commit:75ad23bc0fcb4f992a5d06982bf0857ab1738e9e). I believe patch 3/3 is the intended fix.
---
Dan Williams (3):
md: add new / required locking for calls to blk_remove_plug and blk_plug_queue
md: ping userspace on 'stop' events
md: ping userspace on 'write-pending' events
drivers/md/md.c | 25 +++++++++++++++++++++---- drivers/md/raid1.c | 10 ++++++++-- drivers/md/raid10.c | 10 ++++++++-- drivers/md/raid5.c | 12 ++++++++++-- include/linux/raid/md_k.h | 3 +++ 5 files changed, 50 insertions(+), 10 deletions(-)
Signature · 2 lines
Regards, Dan
PATCH 1/3 md: ping userspace on 'write-pending' events
Also, when userspace writes 'active', clear all mddev->flags to satisfy md_write_start (the other bits do not matter to external-metadata arrays).
---
drivers/md/md.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-)
drivers/md/md.c +15 -4
@@ -2664,7 +2664,7 @@ array_state_store(mddev_t *mddev, const char *buf, size_t len)if (mddev->pers) {restart_array (mddev);if (mddev->external) -clear_bit (MD_CHANGE_CLEAN , &mddev->flags); + mddev->flags =0 ;wake_up (&mddev->sb_wait); err =0 ; }else {
@@ -5413,9 +5413,14 @@ void md_write_start(mddev_t *mddev, struct bio *bi)if (mddev->in_sync) { mddev->in_sync =0 ;set_bit (MD_CHANGE_CLEAN , &mddev->flags); +spin_unlock_irq (&mddev->write_lock); + +/* notify userspace to handle clean->dirty */ +if (mddev->external) +sysfs_notify (&mddev->kobj,NULL ,"array_state" );md_wakeup_thread (mddev->thread); - } -spin_unlock_irq (&mddev->write_lock); + }else +spin_unlock_irq (&mddev->write_lock); }wait_event (mddev->sb_wait, mddev->flags==0 ); }
@@ -5451,7 +5456,13 @@ void md_allow_write(mddev_t *mddev) mddev->safemode == 0) mddev->safemode =1 ;spin_unlock_irq (&mddev->write_lock); -md_update_sb (mddev,0 ); + +/* wait for the dirty state to be recorded in the metadata */ +if (mddev->external) { +sysfs_notify (&mddev->kobj,NULL ,"array_state" ); +wait_event (mddev->sb_wait, mddev->flags ==0 ); + }else +md_update_sb (mddev,0 ); }else spin_unlock_irq (&mddev->write_lock); }
Re: [PATCH 1/3] md: ping userspace on 'write-pending' events
Quoted 4 lines · On Tuesday April 29, [email protected] wrote:
Also, when userspace writes 'active', clear all
mddev->flagsto satisfymd_write_start(the other bits do not matter to external-metadata arrays).Signed-off-by Dan Williams <[email protected]>
Thanks Dan.
There are two things that I don't like about this patch.
1/ I don't think clearing all the flags in array_state_store is really
right. You do that to make sure that MD_CHANGE_DEVS is clear, but
there is no guarantee that it won't be set again before it is tested in md_write_start.
I think it is best to just get md_write_start (and md_allow_write) to just test the bits they are really interested in.
2/ having the tests of mddev->external isn't necessary. I really want
"array_state" to get a notification whenever it changes, no matter what sort of metadata is being used.
Change the patch based on those observations I get:
---------------------------------
### Diffstat output
./drivers/md/md.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/md/md.c +10 -1
@@ -5434,8 +5434,11 @@ void md_write_start(mddev_t *mddev, strumd_wakeup_thread (mddev->thread); }spin_unlock_irq (&mddev->write_lock); +sysfs_notify (&mddev->kobj,NULL ,"array_state" ); } -wait_event (mddev->sb_wait, mddev->flags==0 ); +wait_event (mddev->sb_wait, + !test_bit (MD_CHANGE_CLEAN , &mddev->flags) && + !test_bit (MD_CHANGE_PENDING , &mddev->flags)); }void md_write_end (mddev_t *mddev)
@@ -5470,6 +5473,12 @@ void md_allow_write(mddev_t *mddev) mddev->safemode =1 ;spin_unlock_irq (&mddev->write_lock);md_update_sb (mddev,0 ); + +sysfs_notify (&mddev->kobj,NULL ,"array_state" ); +/* wait for the dirty state to be recorded in the metadata */ +wait_event (mddev->sb_wait, + !test_bit (MD_CHANGE_CLEAN , &mddev->flags) && + !test_bit (MD_CHANGE_PENDING , &mddev->flags)); }else spin_unlock_irq (&mddev->write_lock); }
----------------------------
Look OK ??
Thanks,
NeilBrown
Quoted 51 lines
---
drivers/md/md.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-)drivers/md/md.c +15
-4@@ -2664,7 +2664,7 @@ array_state_store(mddev_t *mddev, const char *buf, size_t len)if (mddev->pers) {restart_array (mddev);if (mddev->external) -clear_bit (MD_CHANGE_CLEAN , &mddev->flags); + mddev->flags =0 ;wake_up (&mddev->sb_wait); err =0 ; }else {@@ -5413,9 +5413,14 @@ void md_write_start(mddev_t *mddev, struct bio *bi)if (mddev->in_sync) { mddev->in_sync =0 ;set_bit (MD_CHANGE_CLEAN , &mddev->flags); +spin_unlock_irq (&mddev->write_lock); + +/* notify userspace to handle clean->dirty */ +if (mddev->external) +sysfs_notify (&mddev->kobj,NULL ,"array_state" );md_wakeup_thread (mddev->thread); - } -spin_unlock_irq (&mddev->write_lock); + }else +spin_unlock_irq (&mddev->write_lock); }wait_event (mddev->sb_wait, mddev->flags==0 ); }@@ -5451,7 +5456,13 @@ void md_allow_write(mddev_t *mddev) mddev->safemode == 0) mddev->safemode =1 ;spin_unlock_irq (&mddev->write_lock); -md_update_sb (mddev,0 ); + +/* wait for the dirty state to be recorded in the metadata */ +if (mddev->external) { +sysfs_notify (&mddev->kobj,NULL ,"array_state" ); +wait_event (mddev->sb_wait, mddev->flags ==0 ); + }else +md_update_sb (mddev,0 ); }else spin_unlock_irq (&mddev->write_lock); }
Re: [PATCH 1/3] md: ping userspace on 'write-pending' events
Quoted 59 lines · Neil Brown
Quoted 4 lines · On Tuesday April 29, [email protected] wrote:
Also, when userspace writes 'active', clear all
mddev->flagsto satisfymd_write_start(the other bits do not matter to external-metadata arrays).Signed-off-by Dan Williams <[email protected]>Thanks Dan.
There are two things that I don't like about this patch.
1/ I don't think clearing all the flags in
array_state_storeis really
right. You do that to make sure thatMD_CHANGE_DEVSis clear, but
there is no guarantee that it won't be set again before it is tested inmd_write_start.
I think it is best to just getmd_write_start(andmd_allow_write) to just test the bits they are really interested in.2/ having the tests of
mddev->externalisn't necessary. I really want
"array_state" to get a notification whenever it changes, no matter what sort of metadata is being used.Change the patch based on those observations I get:
---------------------------------
Signed-off-by Neil Brown <[email protected]>### Diffstat output
./drivers/md/md.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)diff
.prev/drivers/md/md.c./drivers/md/md.c
---.prev/drivers/md/md.c2008-04-29 12:27:58.000000000 +1000
+++./drivers/md/md.c2008-05-01 14:35:16.000000000 +1000
@@ -5434,8 +5434,11 @@ void md_write_start(mddev_t *mddev, strumd_wakeup_thread(mddev->thread); } spin_unlock_irq(&mddev->write_lock);(quoted diff) +3
-0+ sysfs_notify(&mddev->kobj, NULL, "array_state"); } - wait_event(mddev->sb_wait, mddev->flags==0); + wait_event(mddev->sb_wait, + !test_bit(MD_CHANGE_CLEAN, &mddev->flags) && + !test_bit(MD_CHANGE_PENDING, &mddev->flags)); } void md_write_end(mddev_t *mddev) @@ -5470,6 +5473,12 @@ void md_allow_write(mddev_t *mddev) mddev->safemode = 1; spin_unlock_irq(&mddev->write_lock); md_update_sb(mddev, 0); + + sysfs_notify(&mddev->kobj, NULL, "array_state"); + /* wait for the dirty state to be recorded in the metadata */
Review
Quoted 10 lines
(quoted diff) +0
-0+ wait_event(mddev->sb_wait, + !test_bit(MD_CHANGE_CLEAN, &mddev->flags) && + !test_bit(MD_CHANGE_PENDING, &mddev->flags)); } else spin_unlock_irq(&mddev->write_lock); } ---------------------------- Look OK ??
[ thinking out loud here to make sure we are on the same page ]
I had to convince myself that it is still holding off writes while MD_CHANGE_DEVS is set. And yes, MD_CHANGE_PENDING is not cleared until after MD_CHANGE_DEVS is handled. So, the mddev->flags==0 test was indeed more restrictive than it needed to be. Adding the unqualified wait_event() to md_allow_write() also makes sense since it is needed in the 'external' case and should be a nop in the '!external' case since those bits are cleared in md_update_sb().
Simple test confirms this is working as expected.
--
Dan
Re: [PATCH 1/3] md: ping userspace on 'write-pending' events
Quoted 11 lines · On Thursday May 1, [email protected] wrote:
[ thinking out loud here to make sure we are on the same page ]
I had to convince myself that it is still holding off writes while
MD_CHANGE_DEVSis set. And yes,MD_CHANGE_PENDINGis not cleared until afterMD_CHANGE_DEVSis handled. So, the mddev->flags==0 test was indeed more restrictive than it needed to be. Adding the unqualifiedwait_event()tomd_allow_write()also makes sense since it is needed in the 'external' case and should be a nop in the '!external' case since those bits are cleared inmd_update_sb().
Quoted 1 line
Simple test confirms this is working as expected.
Good, thanks.
NeilBrown
PATCH 2/3 md: ping userspace on 'stop' events
In reply to Dan WilliamsThis additional notification to 'array_state' is needed to allow the monitor application to learn about stop events via sysfs. The sysfs_notify("sync_action") call that comes at the end of do_md_stop() (via md_new_event) is insufficient since the 'sync_action' attribute has been removed by this point.
(Seems like a sysfs-notify-on-removal patch is a better fix. Currently removal
updates the event count but does not wake up waiters)
---
drivers/md/md.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/md/md.c +3 -0
@@ -3693,6 +3693,9 @@ static int do_md_stop(mddev_t * mddev, int mode)module_put (mddev->pers->owner); mddev->pers =NULL ; +/* tell userspace to handle 'inactive' */ +if (mddev->external) +sysfs_notify (&mddev->kobj,NULL ,"array_state" );set_capacity (disk,0 ); mddev->changed =1 ;
Re: [PATCH 2/3] md: ping userspace on 'stop' events
Quoted 11 lines · On Tuesday April 29, [email protected] wrote:
This additional notification to '
array_state' is needed to allow the monitor application to learn about stop events via sysfs. Thesysfs_notify("sync_action")call that comes at the end ofdo_md_stop()(viamd_new_event) is insufficient since the 'sync_action' attribute has been removed by this point.(Seems like a sysfs-notify-on-removal patch is a better fix. Currently removal
updates the event count but does not wake up waiters)Signed-off-by Dan Williams <[email protected]>---
Thanks.
Again, drop the "if (mddev->external)". It isn't needed (I have made this change).
NeilBrown
Quoted 19 lines
drivers/md/md.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/md/md.c +3
-0@@ -3693,6 +3693,9 @@ static int do_md_stop(mddev_t * mddev, int mode)module_put (mddev->pers->owner); mddev->pers =NULL ; +/* tell userspace to handle 'inactive' */ +if (mddev->external) +sysfs_notify (&mddev->kobj,NULL ,"array_state" );set_capacity (disk,0 ); mddev->changed =1 ;
PATCH 3/3 md: add new / required locking for calls to blk_remove_plug and blk_plug_queue
In reply to Dan WilliamsNow that queue flags are no longer atomic (commit:75ad23bc0fcb4f992a5d06982bf0857ab1738e9e) we must protect calls to blk_remove_plug with spin_lock(q->queue_lock).
---
drivers/md/md.c | 3 +++ drivers/md/raid1.c | 10 ++++++++-- drivers/md/raid10.c | 10 ++++++++-- drivers/md/raid5.c | 12 ++++++++++-- include/linux/raid/md_k.h | 3 +++ 5 files changed, 32 insertions(+), 6 deletions(-)
drivers/md/md.c +3 -0
@@ -283,6 +283,9 @@ static mddev_t * mddev_find(dev_t unit)kfree (new);return NULL ; } +spin_lock_init (&new->queue_lock); + new->queue->queue_lock = &new->queue_lock; +/* Can be unlocked because the queue is new: no concurrency */ queue_flag_set_unlocked (QUEUE_FLAG_CLUSTER , new->queue);
drivers/md/raid1.c +8 -2
@@ -603,9 +603,13 @@ static int flush_pending_writes(conf_t *conf)spin_lock_irq (&conf->device_lock);if (conf->pending_bio_list.head) { +struct request_queue *q = conf->mddev->queue;struct bio *bio; + bio =bio_list_get (&conf->pending_bio_list); -blk_remove_plug (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_remove_plug (q); +spin_unlock (q->queue_lock);spin_unlock_irq (&conf->device_lock);/* flush any pending bitmap writes to * disk before proceeding w/ I/O */
@@ -964,7 +968,9 @@ static int make_request(struct request_queue *q, struct bio * bio)bio_list_merge (&conf->pending_bio_list, &bl);bio_list_init (&bl); -blk_plug_device (mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock);spin_unlock_irqrestore (&conf->device_lock, flags);/* In case raid1d snuck into freeze_array */
drivers/md/raid10.c +8 -2
@@ -646,9 +646,13 @@ static int flush_pending_writes(conf_t *conf)spin_lock_irq (&conf->device_lock);if (conf->pending_bio_list.head) { +struct request_queue *q = conf->mddev->queue;struct bio *bio; + bio =bio_list_get (&conf->pending_bio_list); -blk_remove_plug (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_remove_plug (q); +spin_unlock (q->queue_lock);spin_unlock_irq (&conf->device_lock);/* flush any pending bitmap writes to disk * before proceeding w/ I/O */
@@ -955,7 +959,9 @@ static int make_request(struct request_queue *q, struct bio * bio)bitmap_startwrite (mddev->bitmap, bio->bi_sector, r10_bio->sectors,0 );spin_lock_irqsave (&conf->device_lock, flags);bio_list_merge (&conf->pending_bio_list, &bl); -blk_plug_device (mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock);spin_unlock_irqrestore (&conf->device_lock, flags);/* In case raid10d snuck in to freeze_array */
drivers/md/raid5.c +10 -2
@@ -124,17 +124,23 @@ static void print_raid5_conf (raid5_conf_t *conf);static void __release_stripe (raid5_conf_t *conf,struct stripe_head *sh) { +struct request_queue *q = conf->mddev->queue; +if (atomic_dec_and_test (&sh->count)) {BUG_ON (!list_empty (&sh->lru));BUG_ON (atomic_read (&conf->active_stripes)==0 );if (test_bit (STRIPE_HANDLE , &sh->state)) {if (test_bit (STRIPE_DELAYED , &sh->state)) {list_add_tail (&sh->lru, &conf->delayed_list); -blk_plug_device (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock); }else if (test_bit (STRIPE_BIT_DELAY , &sh->state) && sh->bm_seq - conf->seq_write >0 ) {list_add_tail (&sh->lru, &conf->bitmap_list); -blk_plug_device (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock); }else {clear_bit (STRIPE_BIT_DELAY , &sh->state);list_add_tail (&sh->lru, &conf->handle_list);
@@ -3268,10 +3274,12 @@ static void raid5_unplug_device(struct request_queue *q)spin_lock_irqsave (&conf->device_lock, flags); +spin_lock (q->queue_lock);if (blk_remove_plug (q)) { conf->seq_flush++;raid5_activate_delayed (conf); } +spin_unlock (q->queue_lock);md_wakeup_thread (mddev->thread);spin_unlock_irqrestore (&conf->device_lock, flags);
include/linux/raid/md_k.h +3 -0
@@ -240,6 +240,9 @@ struct mddev_sstruct timer_list safemode_timer;atomic_t writes_pending;struct request_queue *queue;/* for plugging ... */ +spinlock_t queue_lock;/* we supply the lock +* for blk-core +*/ atomic_t write_behind;/* outstanding async IO */ unsigned int max_write_behind;/* 0 = sync */
Re: [PATCH 3/3] md: add new / required locking for calls to blk_remove_plug and blk_plug_queue
Quoted 3 lines · On Tuesday April 29, [email protected] wrote:
Now that queue flags are no longer atomic (commit:
75ad23bc0fcb4f992a5d06982bf0857ab1738e9e) we must protect calls toblk_remove_plugwithspin_lock(q->queue_lock).
Can't we just do
q->queue_lock = &conf->device_lock
and appropriate places in the various ->run functions?
It seems to be that we are doing appropriate locking, we just need to convince queue_flag_set / queue_flag_clear that the correct lock is locked.
??
NeilBrown
Quoted 143 lines
Signed-off-by Dan Williams <[email protected]>---
drivers/md/md.c | 3 +++ drivers/md/raid1.c | 10 ++++++++-- drivers/md/raid10.c | 10 ++++++++-- drivers/md/raid5.c | 12 ++++++++++-- include/linux/raid/md_k.h | 3 +++ 5 files changed, 32 insertions(+), 6 deletions(-)drivers/md/md.c +3
-0@@ -283,6 +283,9 @@ static mddev_t * mddev_find(dev_t unit)kfree (new);return NULL ; } +spin_lock_init (&new->queue_lock); + new->queue->queue_lock = &new->queue_lock; +/* Can be unlocked because the queue is new: no concurrency */ queue_flag_set_unlocked (QUEUE_FLAG_CLUSTER , new->queue);drivers/md/raid1.c +8
-2@@ -603,9 +603,13 @@ static int flush_pending_writes(conf_t *conf)spin_lock_irq (&conf->device_lock);if (conf->pending_bio_list.head) { +struct request_queue *q = conf->mddev->queue;struct bio *bio; + bio =bio_list_get (&conf->pending_bio_list); -blk_remove_plug (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_remove_plug (q); +spin_unlock (q->queue_lock);spin_unlock_irq (&conf->device_lock);/* flush any pending bitmap writes to * disk before proceeding w/ I/O */ @@ -964,7 +968,9 @@ static int make_request(struct request_queue *q, struct bio * bio)bio_list_merge (&conf->pending_bio_list, &bl);bio_list_init (&bl); -blk_plug_device (mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock);spin_unlock_irqrestore (&conf->device_lock, flags);/* In case raid1d snuck into freeze_array */ drivers/md/raid10.c +8
-2@@ -646,9 +646,13 @@ static int flush_pending_writes(conf_t *conf)spin_lock_irq (&conf->device_lock);if (conf->pending_bio_list.head) { +struct request_queue *q = conf->mddev->queue;struct bio *bio; + bio =bio_list_get (&conf->pending_bio_list); -blk_remove_plug (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_remove_plug (q); +spin_unlock (q->queue_lock);spin_unlock_irq (&conf->device_lock);/* flush any pending bitmap writes to disk * before proceeding w/ I/O */ @@ -955,7 +959,9 @@ static int make_request(struct request_queue *q, struct bio * bio)bitmap_startwrite (mddev->bitmap, bio->bi_sector, r10_bio->sectors,0 );spin_lock_irqsave (&conf->device_lock, flags);bio_list_merge (&conf->pending_bio_list, &bl); -blk_plug_device (mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock);spin_unlock_irqrestore (&conf->device_lock, flags);/* In case raid10d snuck in to freeze_array */ drivers/md/raid5.c +10
-2@@ -124,17 +124,23 @@ static void print_raid5_conf (raid5_conf_t *conf);static void __release_stripe (raid5_conf_t *conf,struct stripe_head *sh) { +struct request_queue *q = conf->mddev->queue; +if (atomic_dec_and_test (&sh->count)) {BUG_ON (!list_empty (&sh->lru));BUG_ON (atomic_read (&conf->active_stripes)==0 );if (test_bit (STRIPE_HANDLE , &sh->state)) {if (test_bit (STRIPE_DELAYED , &sh->state)) {list_add_tail (&sh->lru, &conf->delayed_list); -blk_plug_device (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock); }else if (test_bit (STRIPE_BIT_DELAY , &sh->state) && sh->bm_seq - conf->seq_write >0 ) {list_add_tail (&sh->lru, &conf->bitmap_list); -blk_plug_device (conf->mddev->queue); +spin_lock (q->queue_lock); +blk_plug_device (q); +spin_unlock (q->queue_lock); }else {clear_bit (STRIPE_BIT_DELAY , &sh->state);list_add_tail (&sh->lru, &conf->handle_list);@@ -3268,10 +3274,12 @@ static void raid5_unplug_device(struct request_queue *q)spin_lock_irqsave (&conf->device_lock, flags); +spin_lock (q->queue_lock);if (blk_remove_plug (q)) { conf->seq_flush++;raid5_activate_delayed (conf); } +spin_unlock (q->queue_lock);md_wakeup_thread (mddev->thread);spin_unlock_irqrestore (&conf->device_lock, flags);include/linux/raid/md_k.h +3
-0@@ -240,6 +240,9 @@ struct mddev_sstruct timer_list safemode_timer;atomic_t writes_pending;struct request_queue *queue;/* for plugging ... */ +spinlock_t queue_lock;/* we supply the lock +* for blk-core +*/ atomic_t write_behind;/* outstanding async IO */ unsigned int max_write_behind;/* 0 = sync */
Re: [PATCH 3/3] md: add new / required locking for calls to blk_remove_plug and blk_plug_queue
Quoted 7 lines · Neil Brown
Quoted 3 lines · On Tuesday April 29, [email protected] wrote:
Now that queue flags are no longer atomic (commit:
75ad23bc0fcb4f992a5d06982bf0857ab1738e9e) we must protect calls toblk_remove_plugwithspin_lock(q->queue_lock).Can't we just do
Quoted 9 lines
q->queue_lock=&conf->device_lockand appropriate places in the various ->run functions?
It seems to be that we are doing appropriate locking, we just need to convince
queue_flag_set/queue_flag_clearthat the correct lock is locked.
??
That's much simpler. Hmm... as an additional cleanup should device_lock then move to mddev_t since it really is a mddev-level lock? There seem to be more than a few places that cast mddev->private just to get conf->device_lock. Then all this lock initialization is centralized.
--
Dan
Re: [PATCH 3/3] md: add new / required locking for calls to blk_remove_plug and blk_plug_queue
Quoted 18 lines · On Wednesday April 30, [email protected] wrote:
Quoted 7 lines · Neil Brown
Quoted 3 lines · On Tuesday April 29, [email protected] wrote:
Now that queue flags are no longer atomic (commit:
75ad23bc0fcb4f992a5d06982bf0857ab1738e9e) we must protect calls toblk_remove_plugwithspin_lock(q->queue_lock).Can't we just do
Quoted 9 lines
q->queue_lock=&conf->device_lockand appropriate places in the various ->run functions?
It seems to be that we are doing appropriate locking, we just need to convince
queue_flag_set/queue_flag_clearthat the correct lock is locked.
??
Quoted 5 lines
That's much simpler. Hmm... as an additional cleanup should
device_lockthen move tomddev_tsince it really is a mddev-level lock? There seem to be more than a few places that castmddev->privatejust to getconf->device_lock. Then all this lock initialization is centralized.
Conversely, there seem to be plenty of places where we access conf->device_lock and don't have an 'mddev' handy. They could of course become
conf->mddev->device_lock
but I'm not sure that is an improvement.
I think you would need to actually do the change, then decide if the benefits outweigh the costs.
It's probably best to find a minimal solution to the current problem and just add the assignments to q->queue_lock for now. We can then explore whether there are cleanup opportunities.
Thanks,
NeilBrown