Re: [RFC 4/9] dm: add llseek(SEEK_HOLE/SEEK_DATA) support

[Eric Blake]

On Thu, Mar 28, 2024 at 04:39:05PM -0400, Stefan Hajnoczi wrote:
> Delegate SEEK_HOLE/SEEK_DATA to device-mapper targets. The new
> dm_seek_hole_data() callback allows target types to customize behavior.
> The default implementation treats the target as all data with no holes.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> ---
>  include/linux/device-mapper.h |  5 +++
>  drivers/md/dm.c               | 68 +++++++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)
> 

> +/* Default implementation for targets that do not implement the callback */
> +static loff_t dm_blk_seek_hole_data_default(loff_t offset, int whence,
> +		loff_t size)
> +{
> +	switch (whence) {
> +	case SEEK_DATA:
> +		if ((unsigned long long)offset >= size)
> +			return -ENXIO;
> +		return offset;
> +	case SEEK_HOLE:
> +		if ((unsigned long long)offset >= size)
> +			return -ENXIO;
> +		return size;

These fail with -ENXIO if offset == size (matching what we do on files)...

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static loff_t dm_blk_do_seek_hole_data(struct dm_table *table, loff_t offset,
> +		int whence)
> +{
> +	struct dm_target *ti;
> +	loff_t end;
> +
> +	/* Loop when the end of a target is reached */
> +	do {
> +		ti = dm_table_find_target(table, offset >> SECTOR_SHIFT);
> +		if (!ti)
> +			return whence == SEEK_DATA ? -ENXIO : offset;

..but this blindly returns offset for SEEK_HOLE, even when offset is
beyond the end of the dm.  I think you want 'return -ENXIO;'
unconditionally here.

> +
> +		end = (ti->begin + ti->len) << SECTOR_SHIFT;
> +
> +		if (ti->type->seek_hole_data)
> +			offset = ti->type->seek_hole_data(ti, offset, whence);

Are we guaranteed that ti->type->seek_hole_data will not return a
value exceeding end?  Or can dm be used to truncate the view of an
underlying device, and the underlying seek_hold_data can now return an
answer beyond where dm_table_find_target should look for the next part
of the dm's view?

In which case, should the blkdev_seek_hole_data callback be passed a
max size parameter everywhere, similar to how fixed_size_llseek does
things?

> +		else
> +			offset = dm_blk_seek_hole_data_default(offset, whence, end);
> +
> +		if (whence == SEEK_DATA && offset == -ENXIO)
> +			offset = end;

You have a bug here.  If I have a dm contructed of two underlying targets:

|A  |B  |

and A is all data, then whence == SEEK_HOLE will have offset = -ENXIO
at this point, and you fail to check whether B is also data.  That is,
you have silently treated the rest of the block device as data, which
is semantically not wrong (as that is always a safe fallback), but not
optimal.

I think the correct logic is s/whence == SEEK_DATA &&//.

> +	} while (offset == end);

I'm trying to make sure that we can never return the equivalent of
lseek(dm, 0, SEEK_END).  If you make my above suggested changes, we
will iterate through the do loop once more at EOF, and
dm_table_find_target() will then fail to match at which point we do
get the desired -ENXIO for both SEEK_HOLE and SEEK_DATA.

> +
> +	return offset;
> +}
> +

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org