On Thu, Jan 11 2018 at 2:46am -0500, Hannes Reinecke <hare@xxxxxxx> wrote: > On 01/11/2018 03:12 AM, Mike Snitzer wrote: > > > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c > > index 870484eaed1f..2395122875b4 100644 > > --- a/block/blk-sysfs.c > > +++ b/block/blk-sysfs.c > > @@ -929,6 +930,9 @@ void blk_unregister_queue(struct gendisk *disk) > > if (WARN_ON(!q)) > > return; > > > > + if (!test_bit(QUEUE_FLAG_REGISTERED, &disk->queue->queue_flags)) > > + return; > > + > > mutex_lock(&q->sysfs_lock); > > queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q); > > mutex_unlock(&q->sysfs_lock); > Why can't we use test_and_clear_bit() here? > Shouldn't that relieve the need for the mutex_lock() here, too? FYI, I looked at this and then got concerned with it enough to chat with Mikulas (cc'd) about it, here is what he said: 11:54 <mikulas> if (!test_and_clear_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags)) 11:55 <mikulas> this is buggy - the operation test_and_clear_bit is atomic - but if it races with some non-atomic read-modify-write operation on q->queue_flags, then the atomic modification would be lost 11:56 <mikulas> you must use always atomic accesses on q->queue_flags or always non-atomic accesses inside a mutex 11:56 <mikulas> queue_flag_clear_unlocked uses non-atomic __clear_bit 11:57 <mikulas> other functions in include/linux/blkdev.h also use non-atomic operations - so you cannot mix them with atomic operations So my v4, that I'll send out shortly, won't be using test_and_clear_bit() Thanks, Mike
