Re: [PATCH v4 05/11] block: Add core atomic write support

[John Garry]

On 19/02/2024 22:58, Dave Chinner wrote:
> On Mon, Feb 19, 2024 at 01:01:03PM +0000, John Garry wrote:
> > Add atomic write support as follows:
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index 74e9e775f13d..12a75a252ca2 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -18,6 +18,42 @@
> >   #include "blk-rq-qos.h"
> >   #include "blk-throttle.h"
> >   
> > +static bool rq_straddles_atomic_write_boundary(struct request *rq,
> > +					unsigned int front,
> > +					unsigned int back)
> > +{
> > +	unsigned int boundary = queue_atomic_write_boundary_bytes(rq->q);
> > +	unsigned int mask, imask;
> > +	loff_t start, end;
> > +
> > +	if (!boundary)
> > +		return false;
> > +
> > +	start = rq->__sector << SECTOR_SHIFT;
> > +	end = start + rq->__data_len;
> > +
> > +	start -= front;
> > +	end += back;
> > +
> > +	/* We're longer than the boundary, so must be crossing it */
> > +	if (end - start > boundary)
> > +		return true;
> > +
> > +	mask = boundary - 1;
> > +
> > +	/* start/end are boundary-aligned, so cannot be crossing */
> > +	if (!(start & mask) || !(end & mask))
> > +		return false;
> > +
> > +	imask = ~mask;
> > +
> > +	/* Top bits are different, so crossed a boundary */
> > +	if ((start & imask) != (end & imask))
> > +		return true;
> > +
> > +	return false;
> > +}
> I have no way of verifying this function is doing what it is
> supposed to because it's function is undocumented. I have no idea
> what the front/back variables are supposed to represent, and so no
> clue if they are being applied properly.

I'll add proper function header documentation.

> That said, it's also applying unsigned 32 bit mask variables to
> signed 64 bit quantities and trying to do things like "high bit changed"
> checks on the 64 bit variable. This just smells like a future
> source of "large offsets don't work like we expected!" bugs.

I'll change variables to be unsigned and also all the same size.

> > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > index 06ea91e51b8b..176f26374abc 100644
> > --- a/block/blk-settings.c
> > +++ b/block/blk-settings.c
> > @@ -59,6 +59,13 @@ void blk_set_default_limits(struct queue_limits *lim)
> >   	lim->zoned = false;
> >   	lim->zone_write_granularity = 0;
> >   	lim->dma_alignment = 511;
> > +	lim->atomic_write_hw_max_sectors = 0;
> > +	lim->atomic_write_max_sectors = 0;
> > +	lim->atomic_write_hw_boundary_sectors = 0;
> > +	lim->atomic_write_hw_unit_min_sectors = 0;
> > +	lim->atomic_write_unit_min_sectors = 0;
> > +	lim->atomic_write_hw_unit_max_sectors = 0;
> > +	lim->atomic_write_unit_max_sectors = 0;
> >   }
> Seems to me this function would do better to just
>
> 	memset(lim, 0, sizeof(*lim));
>
> and then set all the non-zero fields.

Christoph responded about limits here and in 
blk_queue_atomic_write_max_bytes(), so please let me know if still some 
concerns.

Thanks,
John