On Thu, Mar 28, 2024 at 07:38:20PM -0500, Eric Blake wrote:
> 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.
If the initial offset is beyond the end of the table, then SEEK_HOLE
should return -ENXIO. I agree that the code doesn't handle this case.
However, returning offset here is correct when there is data at the end
with SEEK_HOLE.
I'll update the code to address the out-of-bounds offset case, perhaps
by checking the initial offset before entering the loop.
>
> > +
> > + 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?
ti->type->seek_hole_data() must not return a value larger than
(ti->begin + ti->len) << SECTOR_SHIFT.
>
> 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 &&//.
No, with whence == SEEK_HOLE and an initial offset in A, the new offset
will be (A->begin + A->end) << SECTOR_SHIFT. The loop will iterate and
continue seeking into B.
The if statement you commented on ensures that we also continue looping
with whence == SEEK_DATA, because that would otherwise prematurely end
with the new offset = -ENXIO.
>
> > + } 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.
Wait, lseek() is supposed to return the equivalent of lseek(dm, 0,
SEEK_END) when whence == SEEK_HOLE and there is data at the end.
>
> > +
> > + return offset;
> > +}
> > +
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
Attachment:
signature.asc
Description: PGP signature
