| Message ID | 20240402130645.653507-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 293066264fb45df4290897c22e69833bea5fe171 |
| Headers | show |
| Series | [01/23] block: add a helper to cancel atomic queue limit updates | expand |
On 4/2/24 22:06, Christoph Hellwig wrote: > Drivers might have to perform complex actions to determine queue limits, > and those might fail. Add a helper to cancel a queue limit update > that can be called in those cases. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
On 4/2/2024 6:36 PM, Christoph Hellwig wrote: > Drivers might have to perform complex actions to determine queue limits, > and those might fail. Add a helper to cancel a queue limit update > that can be called in those cases. > > Signed-off-by: Christoph Hellwig<hch@lst.de> Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
On 4/2/24 15:06, Christoph Hellwig wrote: > Drivers might have to perform complex actions to determine queue limits, > and those might fail. Add a helper to cancel a queue limit update > that can be called in those cases. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/linux/blkdev.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 02/04/2024 14:06, Christoph Hellwig wrote: > Drivers might have to perform complex actions to determine queue limits, > and those might fail. Add a helper to cancel a queue limit update > that can be called in those cases. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/linux/blkdev.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index c3e8f7cf96be9e..ded7f66dc4b964 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -892,6 +892,19 @@ int queue_limits_commit_update(struct request_queue *q, > struct queue_limits *lim); > int queue_limits_set(struct request_queue *q, struct queue_limits *lim); > > +/** > + * queue_limits_cancel_update - cancel an atomic update of queue limits > + * @q: queue to update > + * > + * This functions cancels an atomic update of the queue limits started by > + * queue_limits_start_update() and should be used when an error occurs after > + * starting update. > + */ > +static inline void queue_limits_cancel_update(struct request_queue *q) Just curious, why no __releases() annotation, like what we have in queue_limits_commit_update()? > +{ > + mutex_unlock(&q->limits_lock); > +} > + > /* > * Access functions for manipulating queue properties > */
On Wed, Apr 03, 2024 at 08:38:42AM +0100, John Garry wrote: >> + */ >> +static inline void queue_limits_cancel_update(struct request_queue *q) > > Just curious, why no __releases() annotation, like what we have in > queue_limits_commit_update()? Mostly because sparse doesn't seem to actually need it on inline functins. At least I don't seem to get a sparse warning without it.
On 03/04/2024 13:51, Christoph Hellwig wrote: > On Wed, Apr 03, 2024 at 08:38:42AM +0100, John Garry wrote: >>> + */ >>> +static inline void queue_limits_cancel_update(struct request_queue *q) >> >> Just curious, why no __releases() annotation, like what we have in >> queue_limits_commit_update()? > > Mostly because sparse doesn't seem to actually need it on inline > functins. At least I don't seem to get a sparse warning without it. JFYI, I am noticing this on v6.9-rc2 with vanilla defconfig: john@localhost:~/mnt_sda4/john/linux> make C=1 block/blk-settings.o UPD include/config/kernel.release UPD include/generated/utsrelease.h CALL scripts/checksyscalls.sh DESCEND objtool INSTALL libsubcmd_headers CC block/blk-settings.o CHECK block/blk-settings.c block/blk-settings.c:263:9: warning: context imbalance in 'queue_limits_commit_update' - wrong count at exit john@localhost:~/mnt_sda4/john/linux> Is that a known issue? >
On 4/2/24 06:06, Christoph Hellwig wrote: > Drivers might have to perform complex actions to determine queue limits, > and those might fail. Add a helper to cancel a queue limit update > that can be called in those cases. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/linux/blkdev.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index c3e8f7cf96be9e..ded7f66dc4b964 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -892,6 +892,19 @@ int queue_limits_commit_update(struct request_queue *q, > struct queue_limits *lim); > int queue_limits_set(struct request_queue *q, struct queue_limits *lim); > > +/** > + * queue_limits_cancel_update - cancel an atomic update of queue limits > + * @q: queue to update > + * > + * This functions cancels an atomic update of the queue limits started by > + * queue_limits_start_update() and should be used when an error occurs after > + * starting update. > + */ > +static inline void queue_limits_cancel_update(struct request_queue *q) > +{ > + mutex_unlock(&q->limits_lock); > +} At least in scsi_add_lun() there are multiple statements between queue_limits_start_update(), queue_limits_cancel_update() and queue_limits_commit_update(). Has it been considered to use __cleanup() to invoke queue_limits_commit_update() when the end of the current scope is reached? I think that would make code that uses the queue_limits_*_update() functions easier to verify. For an example of how to use the __cleanup() macro, see e.g. the __free() and no_free_ptr() macros in <linux/cleanup.h>. Thanks, Bart.
On Thu, Apr 04, 2024 at 09:53:03AM -0700, Bart Van Assche wrote: > At least in scsi_add_lun() there are multiple statements between > queue_limits_start_update(), queue_limits_cancel_update() and > queue_limits_commit_update(). Has it been considered to use __cleanup() > to invoke queue_limits_commit_update() when the end of the current scope > is reached? I think that would make code that uses the > queue_limits_*_update() functions easier to verify. For an example of > how to use the __cleanup() macro, see e.g. the __free() and > no_free_ptr() macros in <linux/cleanup.h>. It has been considered and rejected as this syntactic sugar might make teenagers with attention deficit syndrome happy, but otherwise just horribly obfuscates the code.
On Thu, Apr 04, 2024 at 08:14:20AM +0100, John Garry wrote: > > john@localhost:~/mnt_sda4/john/linux> make C=1 block/blk-settings.o > UPD include/config/kernel.release > UPD include/generated/utsrelease.h > CALL scripts/checksyscalls.sh > DESCEND objtool > INSTALL libsubcmd_headers > CC block/blk-settings.o > CHECK block/blk-settings.c > block/blk-settings.c:263:9: warning: context imbalance in > 'queue_limits_commit_update' - wrong count at exit > john@localhost:~/mnt_sda4/john/linux> > > Is that a known issue? I see that too, and it really confuses me as we have the proper annotations there. I'll see what I can do.
On Fri, Apr 05, 2024 at 08:34:18AM +0200, Christoph Hellwig wrote: > > Is that a known issue? > > I see that too, and it really confuses me as we have the proper > annotations there. I'll see what I can do. Seems like sparse lock annotations are really confused by inline function, because if I create an inline wrapper for queue_limits_commit_update that does the locking, that sorts out the warning and removes the need for any annotations. I'll cook up a proper patch for that and will ask Jens if this isn't too ugly.
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index c3e8f7cf96be9e..ded7f66dc4b964 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -892,6 +892,19 @@ int queue_limits_commit_update(struct request_queue *q, struct queue_limits *lim); int queue_limits_set(struct request_queue *q, struct queue_limits *lim); +/** + * queue_limits_cancel_update - cancel an atomic update of queue limits + * @q: queue to update + * + * This functions cancels an atomic update of the queue limits started by + * queue_limits_start_update() and should be used when an error occurs after + * starting update. + */ +static inline void queue_limits_cancel_update(struct request_queue *q) +{ + mutex_unlock(&q->limits_lock); +} + /* * Access functions for manipulating queue properties */
Drivers might have to perform complex actions to determine queue limits, and those might fail. Add a helper to cancel a queue limit update that can be called in those cases. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/blkdev.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)