diff mbox series

[01/23] block: add a helper to cancel atomic queue limit updates

Message ID 20240402130645.653507-2-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/23] block: add a helper to cancel atomic queue limit updates | expand

Commit Message

Christoph Hellwig April 2, 2024, 1:06 p.m. UTC
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(+)

Comments

Damien Le Moal April 2, 2024, 11:24 p.m. UTC | #1
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>
Kanchan Joshi April 3, 2024, 5:04 a.m. UTC | #2
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>
Hannes Reinecke April 3, 2024, 6:45 a.m. UTC | #3
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
John Garry April 3, 2024, 7:38 a.m. UTC | #4
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
>    */
Christoph Hellwig April 3, 2024, 12:51 p.m. UTC | #5
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.
John Garry April 4, 2024, 7:14 a.m. UTC | #6
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?
>
Bart Van Assche April 4, 2024, 4:53 p.m. UTC | #7
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.
Christoph Hellwig April 5, 2024, 6:32 a.m. UTC | #8
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.
Christoph Hellwig April 5, 2024, 6:34 a.m. UTC | #9
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.
Christoph Hellwig April 5, 2024, 6:38 a.m. UTC | #10
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 mbox series

Patch

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
  */