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
