| Message ID | 20240402130645.653507-2-hch@lst.de |
|---|---|
| State | New |
| Headers | show |
| Series | [01/23] block: add a helper to cancel atomic queue limit updates | expand |
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 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 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.
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(+)