Re: [PATCHv8 3/6] block: introduce max_write_hints queue limit

[Keith Busch <kbusch@kernel.org>]

On Fri, Oct 18, 2024 at 09:18:34AM -0700, Bart Van Assche wrote:
> On 10/17/24 9:09 AM, Keith Busch wrote:
> > Drivers with hardware that support write hints need a way to export how
> > many are available so applications can generically query this.
> 
> Something is missing from this patch, namely a change for the SCSI disk
> (sd) driver that sets max_write_hints to sdkp->permanent_stream_count.

Shouldn't someone who cares about scsi do that? I certainly don't care,
nor have I been keeping up with what's happening there, so I'm also
unqualified.
 
> > +What:		/sys/block/<disk>/queue/max_write_hints
> > +Date:		October 2024
> > +Contact:	linux-block@vger.kernel.org
> > +Description:
> > +		[RO] Maximum number of write hints supported, 0 if not
> > +		supported. If supported, valid values are 1 through
> > +		max_write_hints, inclusive.
> 
> That's a bit short. I think it would help to add a reference to the
> aspects of the standards related to this attribute: permanent streams
> for SCSI and FDP for NVMe.

The specs regarding write hints have not historically been stable, so
I'd rather not tie kernel docs to volatile external specifications.

> > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > index a446654ddee5e..921fb4d334fa4 100644
> > --- a/block/blk-settings.c
> > +++ b/block/blk-settings.c
> > @@ -43,6 +43,7 @@ void blk_set_stacking_limits(struct queue_limits *lim)
> >   	lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
> >   	/* Inherit limits from component devices */
> > +	lim->max_write_hints = USHRT_MAX;
> >   	lim->max_segments = USHRT_MAX;
> >   	lim->max_discard_segments = USHRT_MAX;
> >   	lim->max_hw_sectors = UINT_MAX;
> > @@ -544,6 +545,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
> >   	t->max_segment_size = min_not_zero(t->max_segment_size,
> >   					   b->max_segment_size);
> > +	t->max_write_hints = min(t->max_write_hints, b->max_write_hints);
> > +
> >   	alignment = queue_limit_alignment_offset(b, start);
> 
> I prefer that lim->max_write_hints is initialized to zero in
> blk_set_stacking_limits() and that blk_stack_limits() uses
> min_not_zero().

How is a device supposed to report it doesn't support a write hint if 0
gets overridden?