| Message ID | 20211021145918.2691762-3-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | block: keep quiesce & unquiesce balanced for scsi/dm | expand |
On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: > For fixing queue quiesce race between driver and block layer(elevator > switch, update nr_requests, ...), we need to support concurrent > quiesce > and unquiesce, which requires the two call balanced. > > It isn't easy to audit that in all scsi drivers, especially the two > may > be called from different contexts, so do it in scsi core with one > per-device > bit flag & global spinlock, basically zero cost since request queue > quiesce > is seldom triggered. > > Reported-by: Yi Zhang <yi.zhang@redhat.com> > Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue > quiesce/unquiesce") > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++---- > ---- > include/scsi/scsi_device.h | 1 + > 2 files changed, 37 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 51fcd46be265..414f4daf8005 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -2638,6 +2638,40 @@ static int > __scsi_internal_device_block_nowait(struct scsi_device *sdev) > return 0; > } > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock); > + > +void scsi_start_queue(struct scsi_device *sdev) > +{ > + bool need_start; > + unsigned long flags; > + > + spin_lock_irqsave(&sdev_queue_stop_lock, flags); > + need_start = sdev->queue_stopped; > + sdev->queue_stopped = 0; > + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); > + > + if (need_start) > + blk_mq_unquiesce_queue(sdev->request_queue); Well, this is a classic atomic pattern: if (cmpxchg(&sdev->queue_stopped, 1, 0)) blk_mq_unquiesce_queue(sdev->request_queue); The reason to do it with atomics rather than spinlocks is 1. no need to disable interrupts: atomics are locked 2. faster because a spinlock takes an exclusive line every time but the read to check the value can be in shared mode in cmpxchg 3. it's just shorter and better code. The only minor downside is queue_stopped now needs to be a u32. James -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi James, On Mon, Nov 01, 2021 at 09:43:27PM -0400, James Bottomley wrote: > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: > > For fixing queue quiesce race between driver and block layer(elevator > > switch, update nr_requests, ...), we need to support concurrent > > quiesce > > and unquiesce, which requires the two call balanced. > > > > It isn't easy to audit that in all scsi drivers, especially the two > > may > > be called from different contexts, so do it in scsi core with one > > per-device > > bit flag & global spinlock, basically zero cost since request queue > > quiesce > > is seldom triggered. > > > > Reported-by: Yi Zhang <yi.zhang@redhat.com> > > Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue > > quiesce/unquiesce") > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++---- > > ---- > > include/scsi/scsi_device.h | 1 + > > 2 files changed, 37 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > index 51fcd46be265..414f4daf8005 100644 > > --- a/drivers/scsi/scsi_lib.c > > +++ b/drivers/scsi/scsi_lib.c > > @@ -2638,6 +2638,40 @@ static int > > __scsi_internal_device_block_nowait(struct scsi_device *sdev) > > return 0; > > } > > > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock); > > + > > +void scsi_start_queue(struct scsi_device *sdev) > > +{ > > + bool need_start; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&sdev_queue_stop_lock, flags); > > + need_start = sdev->queue_stopped; > > + sdev->queue_stopped = 0; > > + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); > > + > > + if (need_start) > > + blk_mq_unquiesce_queue(sdev->request_queue); > > Well, this is a classic atomic pattern: > > if (cmpxchg(&sdev->queue_stopped, 1, 0)) > blk_mq_unquiesce_queue(sdev->request_queue); > > The reason to do it with atomics rather than spinlocks is > > 1. no need to disable interrupts: atomics are locked > 2. faster because a spinlock takes an exclusive line every time but the > read to check the value can be in shared mode in cmpxchg > 3. it's just shorter and better code. You are right, I agree. > > The only minor downside is queue_stopped now needs to be a u32. Yeah, that is the reason I don't take this atomic way since it needs to add one extra u32 into 'struct scsi_device'. Thanks, Ming -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 11/1/21 7:43 PM, James Bottomley wrote: > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: >> For fixing queue quiesce race between driver and block layer(elevator >> switch, update nr_requests, ...), we need to support concurrent >> quiesce >> and unquiesce, which requires the two call balanced. >> >> It isn't easy to audit that in all scsi drivers, especially the two >> may >> be called from different contexts, so do it in scsi core with one >> per-device >> bit flag & global spinlock, basically zero cost since request queue >> quiesce >> is seldom triggered. >> >> Reported-by: Yi Zhang <yi.zhang@redhat.com> >> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue >> quiesce/unquiesce") >> Signed-off-by: Ming Lei <ming.lei@redhat.com> >> --- >> drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++---- >> ---- >> include/scsi/scsi_device.h | 1 + >> 2 files changed, 37 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 51fcd46be265..414f4daf8005 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -2638,6 +2638,40 @@ static int >> __scsi_internal_device_block_nowait(struct scsi_device *sdev) >> return 0; >> } >> >> +static DEFINE_SPINLOCK(sdev_queue_stop_lock); >> + >> +void scsi_start_queue(struct scsi_device *sdev) >> +{ >> + bool need_start; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&sdev_queue_stop_lock, flags); >> + need_start = sdev->queue_stopped; >> + sdev->queue_stopped = 0; >> + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); >> + >> + if (need_start) >> + blk_mq_unquiesce_queue(sdev->request_queue); > > Well, this is a classic atomic pattern: > > if (cmpxchg(&sdev->queue_stopped, 1, 0)) > blk_mq_unquiesce_queue(sdev->request_queue); > > The reason to do it with atomics rather than spinlocks is > > 1. no need to disable interrupts: atomics are locked > 2. faster because a spinlock takes an exclusive line every time but the > read to check the value can be in shared mode in cmpxchg > 3. it's just shorter and better code. > > The only minor downside is queue_stopped now needs to be a u32. Are you fine with the change as-is, or do you want it redone? I can drop the SCSI parts and just queue up the dm fix. Personally I think it'd be better to get it fixed upfront.
On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote: > On 11/1/21 7:43 PM, James Bottomley wrote: > > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: > > > For fixing queue quiesce race between driver and block > > > layer(elevator switch, update nr_requests, ...), we need to > > > support concurrent quiesce and unquiesce, which requires the two > > > call balanced. > > > > > > It isn't easy to audit that in all scsi drivers, especially the > > > two may be called from different contexts, so do it in scsi core > > > with one per-device bit flag & global spinlock, basically zero > > > cost since request queue quiesce is seldom triggered. > > > > > > Reported-by: Yi Zhang <yi.zhang@redhat.com> > > > Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue > > > quiesce/unquiesce") > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > > --- > > > drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++ > > > ---- > > > ---- > > > include/scsi/scsi_device.h | 1 + > > > 2 files changed, 37 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > > index 51fcd46be265..414f4daf8005 100644 > > > --- a/drivers/scsi/scsi_lib.c > > > +++ b/drivers/scsi/scsi_lib.c > > > @@ -2638,6 +2638,40 @@ static int > > > __scsi_internal_device_block_nowait(struct scsi_device *sdev) > > > return 0; > > > } > > > > > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock); > > > + > > > +void scsi_start_queue(struct scsi_device *sdev) > > > +{ > > > + bool need_start; > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&sdev_queue_stop_lock, flags); > > > + need_start = sdev->queue_stopped; > > > + sdev->queue_stopped = 0; > > > + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); > > > + > > > + if (need_start) > > > + blk_mq_unquiesce_queue(sdev->request_queue); > > > > Well, this is a classic atomic pattern: > > > > if (cmpxchg(&sdev->queue_stopped, 1, 0)) > > blk_mq_unquiesce_queue(sdev->request_queue); > > > > The reason to do it with atomics rather than spinlocks is > > > > 1. no need to disable interrupts: atomics are locked > > 2. faster because a spinlock takes an exclusive line every time > > but the > > read to check the value can be in shared mode in cmpxchg > > 3. it's just shorter and better code. > > > > The only minor downside is queue_stopped now needs to be a u32. > > Are you fine with the change as-is, or do you want it redone? I > can drop the SCSI parts and just queue up the dm fix. Personally > I think it'd be better to get it fixed upfront. Well, given the path isn't hot, I don't really care. However, what I don't want is to have to continually bat back patches from the make work code churners trying to update this code for being the wrong pattern. I think at the very least it needs a comment saying why we chose a suboptimal pattern to try to forestall this. James -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 11/2/21 8:33 AM, James Bottomley wrote: > On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote: >> On 11/1/21 7:43 PM, James Bottomley wrote: >>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: >>>> For fixing queue quiesce race between driver and block >>>> layer(elevator switch, update nr_requests, ...), we need to >>>> support concurrent quiesce and unquiesce, which requires the two >>>> call balanced. >>>> >>>> It isn't easy to audit that in all scsi drivers, especially the >>>> two may be called from different contexts, so do it in scsi core >>>> with one per-device bit flag & global spinlock, basically zero >>>> cost since request queue quiesce is seldom triggered. >>>> >>>> Reported-by: Yi Zhang <yi.zhang@redhat.com> >>>> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue >>>> quiesce/unquiesce") >>>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>>> --- >>>> drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++ >>>> ---- >>>> ---- >>>> include/scsi/scsi_device.h | 1 + >>>> 2 files changed, 37 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >>>> index 51fcd46be265..414f4daf8005 100644 >>>> --- a/drivers/scsi/scsi_lib.c >>>> +++ b/drivers/scsi/scsi_lib.c >>>> @@ -2638,6 +2638,40 @@ static int >>>> __scsi_internal_device_block_nowait(struct scsi_device *sdev) >>>> return 0; >>>> } >>>> >>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock); >>>> + >>>> +void scsi_start_queue(struct scsi_device *sdev) >>>> +{ >>>> + bool need_start; >>>> + unsigned long flags; >>>> + >>>> + spin_lock_irqsave(&sdev_queue_stop_lock, flags); >>>> + need_start = sdev->queue_stopped; >>>> + sdev->queue_stopped = 0; >>>> + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); >>>> + >>>> + if (need_start) >>>> + blk_mq_unquiesce_queue(sdev->request_queue); >>> >>> Well, this is a classic atomic pattern: >>> >>> if (cmpxchg(&sdev->queue_stopped, 1, 0)) >>> blk_mq_unquiesce_queue(sdev->request_queue); >>> >>> The reason to do it with atomics rather than spinlocks is >>> >>> 1. no need to disable interrupts: atomics are locked >>> 2. faster because a spinlock takes an exclusive line every time >>> but the >>> read to check the value can be in shared mode in cmpxchg >>> 3. it's just shorter and better code. >>> >>> The only minor downside is queue_stopped now needs to be a u32. >> >> Are you fine with the change as-is, or do you want it redone? I >> can drop the SCSI parts and just queue up the dm fix. Personally >> I think it'd be better to get it fixed upfront. > > Well, given the path isn't hot, I don't really care. However, what I > don't want is to have to continually bat back patches from the make > work code churners trying to update this code for being the wrong > pattern. I think at the very least it needs a comment saying why we > chose a suboptimal pattern to try to forestall this. Right, with a comment it's probably better. And as you said, since it's not a hot path, don't think we'd be revisiting it anyway. I'll amend the patch with a comment.
On 11/2/21 8:36 AM, Jens Axboe wrote: > On 11/2/21 8:33 AM, James Bottomley wrote: >> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote: >>> On 11/1/21 7:43 PM, James Bottomley wrote: >>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: >>>>> For fixing queue quiesce race between driver and block >>>>> layer(elevator switch, update nr_requests, ...), we need to >>>>> support concurrent quiesce and unquiesce, which requires the two >>>>> call balanced. >>>>> >>>>> It isn't easy to audit that in all scsi drivers, especially the >>>>> two may be called from different contexts, so do it in scsi core >>>>> with one per-device bit flag & global spinlock, basically zero >>>>> cost since request queue quiesce is seldom triggered. >>>>> >>>>> Reported-by: Yi Zhang <yi.zhang@redhat.com> >>>>> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue >>>>> quiesce/unquiesce") >>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>>>> --- >>>>> drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++ >>>>> ---- >>>>> ---- >>>>> include/scsi/scsi_device.h | 1 + >>>>> 2 files changed, 37 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >>>>> index 51fcd46be265..414f4daf8005 100644 >>>>> --- a/drivers/scsi/scsi_lib.c >>>>> +++ b/drivers/scsi/scsi_lib.c >>>>> @@ -2638,6 +2638,40 @@ static int >>>>> __scsi_internal_device_block_nowait(struct scsi_device *sdev) >>>>> return 0; >>>>> } >>>>> >>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock); >>>>> + >>>>> +void scsi_start_queue(struct scsi_device *sdev) >>>>> +{ >>>>> + bool need_start; >>>>> + unsigned long flags; >>>>> + >>>>> + spin_lock_irqsave(&sdev_queue_stop_lock, flags); >>>>> + need_start = sdev->queue_stopped; >>>>> + sdev->queue_stopped = 0; >>>>> + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); >>>>> + >>>>> + if (need_start) >>>>> + blk_mq_unquiesce_queue(sdev->request_queue); >>>> >>>> Well, this is a classic atomic pattern: >>>> >>>> if (cmpxchg(&sdev->queue_stopped, 1, 0)) >>>> blk_mq_unquiesce_queue(sdev->request_queue); >>>> >>>> The reason to do it with atomics rather than spinlocks is >>>> >>>> 1. no need to disable interrupts: atomics are locked >>>> 2. faster because a spinlock takes an exclusive line every time >>>> but the >>>> read to check the value can be in shared mode in cmpxchg >>>> 3. it's just shorter and better code. >>>> >>>> The only minor downside is queue_stopped now needs to be a u32. >>> >>> Are you fine with the change as-is, or do you want it redone? I >>> can drop the SCSI parts and just queue up the dm fix. Personally >>> I think it'd be better to get it fixed upfront. >> >> Well, given the path isn't hot, I don't really care. However, what I >> don't want is to have to continually bat back patches from the make >> work code churners trying to update this code for being the wrong >> pattern. I think at the very least it needs a comment saying why we >> chose a suboptimal pattern to try to forestall this. > > Right, with a comment it's probably better. And as you said, since it's > not a hot path, don't think we'd be revisiting it anyway. > > I'll amend the patch with a comment. I started adding the comment and took another look at this, and that made me change my mind. We really should make this a cmpxcgh, it's not even using a device lock here. I've dropped the two SCSI patches for now, Ming can you resend? If James agrees, I really think queue_stopped should just have the type changed and the patch redone with that using cmpxcgh().
On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote: > On 11/2/21 8:36 AM, Jens Axboe wrote: > > On 11/2/21 8:33 AM, James Bottomley wrote: > > > On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote: > > > > On 11/1/21 7:43 PM, James Bottomley wrote: > > > > > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: > > > > > > For fixing queue quiesce race between driver and block > > > > > > layer(elevator switch, update nr_requests, ...), we need to > > > > > > support concurrent quiesce and unquiesce, which requires > > > > > > the two > > > > > > call balanced. > > > > > > > > > > > > It isn't easy to audit that in all scsi drivers, especially > > > > > > the two may be called from different contexts, so do it in > > > > > > scsi core with one per-device bit flag & global spinlock, > > > > > > basically zero cost since request queue quiesce is seldom > > > > > > triggered. > > > > > > > > > > > > Reported-by: Yi Zhang <yi.zhang@redhat.com> > > > > > > Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue > > > > > > quiesce/unquiesce") > > > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > > > > > --- > > > > > > drivers/scsi/scsi_lib.c | 45 > > > > > > ++++++++++++++++++++++++++++++ > > > > > > ---- > > > > > > ---- > > > > > > include/scsi/scsi_device.h | 1 + > > > > > > 2 files changed, 37 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/drivers/scsi/scsi_lib.c > > > > > > b/drivers/scsi/scsi_lib.c > > > > > > index 51fcd46be265..414f4daf8005 100644 > > > > > > --- a/drivers/scsi/scsi_lib.c > > > > > > +++ b/drivers/scsi/scsi_lib.c > > > > > > @@ -2638,6 +2638,40 @@ static int > > > > > > __scsi_internal_device_block_nowait(struct scsi_device > > > > > > *sdev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock); > > > > > > + > > > > > > +void scsi_start_queue(struct scsi_device *sdev) > > > > > > +{ > > > > > > + bool need_start; > > > > > > + unsigned long flags; > > > > > > + > > > > > > + spin_lock_irqsave(&sdev_queue_stop_lock, flags); > > > > > > + need_start = sdev->queue_stopped; > > > > > > + sdev->queue_stopped = 0; > > > > > > + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); > > > > > > + > > > > > > + if (need_start) > > > > > > + blk_mq_unquiesce_queue(sdev->request_queue); > > > > > > > > > > Well, this is a classic atomic pattern: > > > > > > > > > > if (cmpxchg(&sdev->queue_stopped, 1, 0)) > > > > > blk_mq_unquiesce_queue(sdev->request_queue); > > > > > > > > > > The reason to do it with atomics rather than spinlocks is > > > > > > > > > > 1. no need to disable interrupts: atomics are locked > > > > > 2. faster because a spinlock takes an exclusive line every > > > > > time but the > > > > > read to check the value can be in shared mode in > > > > > cmpxchg > > > > > 3. it's just shorter and better code. > > > > > > > > > > The only minor downside is queue_stopped now needs to be a > > > > > u32. > > > > > > > > Are you fine with the change as-is, or do you want it redone? I > > > > can drop the SCSI parts and just queue up the dm fix. > > > > Personally I think it'd be better to get it fixed upfront. > > > > > > Well, given the path isn't hot, I don't really care. However, > > > what I don't want is to have to continually bat back patches from > > > the make work code churners trying to update this code for being > > > the wrong pattern. I think at the very least it needs a comment > > > saying why we chose a suboptimal pattern to try to forestall > > > this. > > > > Right, with a comment it's probably better. And as you said, since > > it's not a hot path, don't think we'd be revisiting it anyway. > > > > I'll amend the patch with a comment. > > I started adding the comment and took another look at this, and that > made me change my mind. We really should make this a cmpxcgh, it's > not even using a device lock here. > > I've dropped the two SCSI patches for now, Ming can you resend? If > James agrees, I really think queue_stopped should just have the type > changed and the patch redone with that using cmpxcgh(). Well, that's what I suggested originally, so I agree ... I don't think 31 more bytes is going to be a huge burden to scsi_device. James -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 11/2/21 8:47 AM, James Bottomley wrote: > On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote: >> On 11/2/21 8:36 AM, Jens Axboe wrote: >>> On 11/2/21 8:33 AM, James Bottomley wrote: >>>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote: >>>>> On 11/1/21 7:43 PM, James Bottomley wrote: >>>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: >>>>>>> For fixing queue quiesce race between driver and block >>>>>>> layer(elevator switch, update nr_requests, ...), we need to >>>>>>> support concurrent quiesce and unquiesce, which requires >>>>>>> the two >>>>>>> call balanced. >>>>>>> >>>>>>> It isn't easy to audit that in all scsi drivers, especially >>>>>>> the two may be called from different contexts, so do it in >>>>>>> scsi core with one per-device bit flag & global spinlock, >>>>>>> basically zero cost since request queue quiesce is seldom >>>>>>> triggered. >>>>>>> >>>>>>> Reported-by: Yi Zhang <yi.zhang@redhat.com> >>>>>>> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue >>>>>>> quiesce/unquiesce") >>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>>>>>> --- >>>>>>> drivers/scsi/scsi_lib.c | 45 >>>>>>> ++++++++++++++++++++++++++++++ >>>>>>> ---- >>>>>>> ---- >>>>>>> include/scsi/scsi_device.h | 1 + >>>>>>> 2 files changed, 37 insertions(+), 9 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/scsi/scsi_lib.c >>>>>>> b/drivers/scsi/scsi_lib.c >>>>>>> index 51fcd46be265..414f4daf8005 100644 >>>>>>> --- a/drivers/scsi/scsi_lib.c >>>>>>> +++ b/drivers/scsi/scsi_lib.c >>>>>>> @@ -2638,6 +2638,40 @@ static int >>>>>>> __scsi_internal_device_block_nowait(struct scsi_device >>>>>>> *sdev) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock); >>>>>>> + >>>>>>> +void scsi_start_queue(struct scsi_device *sdev) >>>>>>> +{ >>>>>>> + bool need_start; >>>>>>> + unsigned long flags; >>>>>>> + >>>>>>> + spin_lock_irqsave(&sdev_queue_stop_lock, flags); >>>>>>> + need_start = sdev->queue_stopped; >>>>>>> + sdev->queue_stopped = 0; >>>>>>> + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); >>>>>>> + >>>>>>> + if (need_start) >>>>>>> + blk_mq_unquiesce_queue(sdev->request_queue); >>>>>> >>>>>> Well, this is a classic atomic pattern: >>>>>> >>>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0)) >>>>>> blk_mq_unquiesce_queue(sdev->request_queue); >>>>>> >>>>>> The reason to do it with atomics rather than spinlocks is >>>>>> >>>>>> 1. no need to disable interrupts: atomics are locked >>>>>> 2. faster because a spinlock takes an exclusive line every >>>>>> time but the >>>>>> read to check the value can be in shared mode in >>>>>> cmpxchg >>>>>> 3. it's just shorter and better code. >>>>>> >>>>>> The only minor downside is queue_stopped now needs to be a >>>>>> u32. >>>>> >>>>> Are you fine with the change as-is, or do you want it redone? I >>>>> can drop the SCSI parts and just queue up the dm fix. >>>>> Personally I think it'd be better to get it fixed upfront. >>>> >>>> Well, given the path isn't hot, I don't really care. However, >>>> what I don't want is to have to continually bat back patches from >>>> the make work code churners trying to update this code for being >>>> the wrong pattern. I think at the very least it needs a comment >>>> saying why we chose a suboptimal pattern to try to forestall >>>> this. >>> >>> Right, with a comment it's probably better. And as you said, since >>> it's not a hot path, don't think we'd be revisiting it anyway. >>> >>> I'll amend the patch with a comment. >> >> I started adding the comment and took another look at this, and that >> made me change my mind. We really should make this a cmpxcgh, it's >> not even using a device lock here. >> >> I've dropped the two SCSI patches for now, Ming can you resend? If >> James agrees, I really think queue_stopped should just have the type >> changed and the patch redone with that using cmpxcgh(). > > Well, that's what I suggested originally, so I agree ... I don't think > 31 more bytes is going to be a huge burden to scsi_device. Yeah I know, just saying I agree with that being the better solution.
On 11/2/21 8:47 AM, James Bottomley wrote: > On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote: >> On 11/2/21 8:36 AM, Jens Axboe wrote: >>> On 11/2/21 8:33 AM, James Bottomley wrote: >>>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote: >>>>> On 11/1/21 7:43 PM, James Bottomley wrote: >>>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote: >>>>>>> For fixing queue quiesce race between driver and block >>>>>>> layer(elevator switch, update nr_requests, ...), we need to >>>>>>> support concurrent quiesce and unquiesce, which requires >>>>>>> the two >>>>>>> call balanced. >>>>>>> >>>>>>> It isn't easy to audit that in all scsi drivers, especially >>>>>>> the two may be called from different contexts, so do it in >>>>>>> scsi core with one per-device bit flag & global spinlock, >>>>>>> basically zero cost since request queue quiesce is seldom >>>>>>> triggered. >>>>>>> >>>>>>> Reported-by: Yi Zhang <yi.zhang@redhat.com> >>>>>>> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue >>>>>>> quiesce/unquiesce") >>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>>>>>> --- >>>>>>> drivers/scsi/scsi_lib.c | 45 >>>>>>> ++++++++++++++++++++++++++++++ >>>>>>> ---- >>>>>>> ---- >>>>>>> include/scsi/scsi_device.h | 1 + >>>>>>> 2 files changed, 37 insertions(+), 9 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/scsi/scsi_lib.c >>>>>>> b/drivers/scsi/scsi_lib.c >>>>>>> index 51fcd46be265..414f4daf8005 100644 >>>>>>> --- a/drivers/scsi/scsi_lib.c >>>>>>> +++ b/drivers/scsi/scsi_lib.c >>>>>>> @@ -2638,6 +2638,40 @@ static int >>>>>>> __scsi_internal_device_block_nowait(struct scsi_device >>>>>>> *sdev) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock); >>>>>>> + >>>>>>> +void scsi_start_queue(struct scsi_device *sdev) >>>>>>> +{ >>>>>>> + bool need_start; >>>>>>> + unsigned long flags; >>>>>>> + >>>>>>> + spin_lock_irqsave(&sdev_queue_stop_lock, flags); >>>>>>> + need_start = sdev->queue_stopped; >>>>>>> + sdev->queue_stopped = 0; >>>>>>> + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); >>>>>>> + >>>>>>> + if (need_start) >>>>>>> + blk_mq_unquiesce_queue(sdev->request_queue); >>>>>> >>>>>> Well, this is a classic atomic pattern: >>>>>> >>>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0)) >>>>>> blk_mq_unquiesce_queue(sdev->request_queue); >>>>>> >>>>>> The reason to do it with atomics rather than spinlocks is >>>>>> >>>>>> 1. no need to disable interrupts: atomics are locked >>>>>> 2. faster because a spinlock takes an exclusive line every >>>>>> time but the >>>>>> read to check the value can be in shared mode in >>>>>> cmpxchg >>>>>> 3. it's just shorter and better code. >>>>>> >>>>>> The only minor downside is queue_stopped now needs to be a >>>>>> u32. >>>>> >>>>> Are you fine with the change as-is, or do you want it redone? I >>>>> can drop the SCSI parts and just queue up the dm fix. >>>>> Personally I think it'd be better to get it fixed upfront. >>>> >>>> Well, given the path isn't hot, I don't really care. However, >>>> what I don't want is to have to continually bat back patches from >>>> the make work code churners trying to update this code for being >>>> the wrong pattern. I think at the very least it needs a comment >>>> saying why we chose a suboptimal pattern to try to forestall >>>> this. >>> >>> Right, with a comment it's probably better. And as you said, since >>> it's not a hot path, don't think we'd be revisiting it anyway. >>> >>> I'll amend the patch with a comment. >> >> I started adding the comment and took another look at this, and that >> made me change my mind. We really should make this a cmpxcgh, it's >> not even using a device lock here. >> >> I've dropped the two SCSI patches for now, Ming can you resend? If >> James agrees, I really think queue_stopped should just have the type >> changed and the patch redone with that using cmpxcgh(). > > Well, that's what I suggested originally, so I agree ... I don't think > 31 more bytes is going to be a huge burden to scsi_device. ^^^^ Bits? :-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 51fcd46be265..414f4daf8005 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2638,6 +2638,40 @@ static int __scsi_internal_device_block_nowait(struct scsi_device *sdev) return 0; } +static DEFINE_SPINLOCK(sdev_queue_stop_lock); + +void scsi_start_queue(struct scsi_device *sdev) +{ + bool need_start; + unsigned long flags; + + spin_lock_irqsave(&sdev_queue_stop_lock, flags); + need_start = sdev->queue_stopped; + sdev->queue_stopped = 0; + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); + + if (need_start) + blk_mq_unquiesce_queue(sdev->request_queue); +} + +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) +{ + bool need_stop; + unsigned long flags; + + spin_lock_irqsave(&sdev_queue_stop_lock, flags); + need_stop = !sdev->queue_stopped; + sdev->queue_stopped = 1; + spin_unlock_irqrestore(&sdev_queue_stop_lock, flags); + + if (need_stop) { + if (nowait) + blk_mq_quiesce_queue_nowait(sdev->request_queue); + else + blk_mq_quiesce_queue(sdev->request_queue); + } +} + /** * scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state * @sdev: device to block @@ -2662,7 +2696,7 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev) * request queue. */ if (!ret) - blk_mq_quiesce_queue_nowait(sdev->request_queue); + scsi_stop_queue(sdev, true); return ret; } EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait); @@ -2689,19 +2723,12 @@ static int scsi_internal_device_block(struct scsi_device *sdev) mutex_lock(&sdev->state_mutex); err = __scsi_internal_device_block_nowait(sdev); if (err == 0) - blk_mq_quiesce_queue(sdev->request_queue); + scsi_stop_queue(sdev, false); mutex_unlock(&sdev->state_mutex); return err; } -void scsi_start_queue(struct scsi_device *sdev) -{ - struct request_queue *q = sdev->request_queue; - - blk_mq_unquiesce_queue(q); -} - /** * scsi_internal_device_unblock_nowait - resume a device after a block request * @sdev: device to resume diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 430b73bd02ac..ac74beccffa2 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -206,6 +206,7 @@ struct scsi_device { unsigned rpm_autosuspend:1; /* Enable runtime autosuspend at device * creation time */ unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */ + unsigned queue_stopped:1; /* request queue is quiesced */ bool offline_already; /* Device offline message logged */
For fixing queue quiesce race between driver and block layer(elevator switch, update nr_requests, ...), we need to support concurrent quiesce and unquiesce, which requires the two call balanced. It isn't easy to audit that in all scsi drivers, especially the two may be called from different contexts, so do it in scsi core with one per-device bit flag & global spinlock, basically zero cost since request queue quiesce is seldom triggered. Reported-by: Yi Zhang <yi.zhang@redhat.com> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue quiesce/unquiesce") Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/scsi_lib.c | 45 ++++++++++++++++++++++++++++++-------- include/scsi/scsi_device.h | 1 + 2 files changed, 37 insertions(+), 9 deletions(-)