Received-SPF: pass (google.com: domain of linux-kernel+bounces-130403-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 3 Apr 2024 13:58:38 -0400
From: Stefan Hajnoczi <stefanha@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	Alasdair Kergon <agk@redhat.com>,
	Mikulas Patocka <mpatocka@redhat.com>, dm-devel@lists.linux.dev,
	David Teigland <teigland@redhat.com>,
	Mike Snitzer <snitzer@kernel.org>, Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@lst.de>, Joe Thornber <ejt@redhat.com>
Subject: Re: [RFC 4/9] dm: add llseek(SEEK_HOLE/SEEK_DATA) support
Message-ID: <20240403175838.GB2534900@fedora>
References: <20240328203910.2370087-1-stefanha@redhat.com>
 <20240328203910.2370087-5-stefanha@redhat.com>
 <6awt5gq36kzwhuobabtye5vhnexc6cufuamy4frymehuv57ky5@esel3f5naqyu>
 <20240403141147.GD2524049@fedora>
 <mi3yp4kel6junjk2corv4hi56s56pmwilnm2bb4gg2tbbvyq2n@zmzaqpdq2rlq>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="N31yZm+DVUhVXgFt"
Content-Disposition: inline
In-Reply-To: <mi3yp4kel6junjk2corv4hi56s56pmwilnm2bb4gg2tbbvyq2n@zmzaqpdq2rlq>


--N31yZm+DVUhVXgFt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 03, 2024 at 12:02:19PM -0500, Eric Blake wrote:
> On Wed, Apr 03, 2024 at 10:11:47AM -0400, Stefan Hajnoczi wrote:
> ...
> > > > +static loff_t dm_blk_do_seek_hole_data(struct dm_table *table, lof=
f_t offset,
> > > > +		int whence)
> > > > +{
> > > > +	struct dm_target *ti;
> > > > +	loff_t end;
> > > > +
> > > > +	/* Loop when the end of a target is reached */
> > > > +	do {
> > > > +		ti =3D dm_table_find_target(table, offset >> SECTOR_SHIFT);
> > > > +		if (!ti)
> > > > +			return whence =3D=3D SEEK_DATA ? -ENXIO : offset;
> > >=20
> > > ...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.
> >=20
> > 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.
> >=20
> > However, returning offset here is correct when there is data at the end
> > with SEEK_HOLE.
> >=20
> > I'll update the code to address the out-of-bounds offset case, perhaps
> > by checking the initial offset before entering the loop.
>=20
> You are correct that if we are on the second loop iteration of
> SEEK_HOLE (because the first iteration saw all data), then we have
> found the offset of the start of a hole and should return that offset,
> not -ENXIO.  This may be a case where we just have to be careful on
> whether the initial pass might have any corner cases different from
> later times through the loop, and that we end the loop with correct
> results for both SEEK_HOLE and SEEK_DATA.
>=20
> >=20
> > >=20
> > > > +
> > > > +		end =3D (ti->begin + ti->len) << SECTOR_SHIFT;
> > > > +
> > > > +		if (ti->type->seek_hole_data)
> > > > +			offset =3D ti->type->seek_hole_data(ti, offset, whence);
> > >=20
> > > 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?
> >=20
> > ti->type->seek_hole_data() must not return a value larger than
> > (ti->begin + ti->len) << SECTOR_SHIFT.
>=20
> Worth adding as documentation then.
>=20
> >=20
> > >=20
> > > 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?
> > >=20
> > > > +		else
> > > > +			offset =3D dm_blk_seek_hole_data_default(offset, whence, end);
> > > > +
> > > > +		if (whence =3D=3D SEEK_DATA && offset =3D=3D -ENXIO)
> > > > +			offset =3D end;
> > >=20
> > > You have a bug here.  If I have a dm contructed of two underlying tar=
gets:
> > >=20
> > > |A  |B  |
> > >=20
> > > and A is all data, then whence =3D=3D SEEK_HOLE will have offset =3D =
-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.
> > >=20
> > > I think the correct logic is s/whence =3D=3D SEEK_DATA &&//.
> >=20
> > No, with whence =3D=3D SEEK_HOLE and an initial offset in A, the new of=
fset
> > will be (A->begin + A->end) << SECTOR_SHIFT. The loop will iterate and
> > continue seeking into B.
> >=20
> > The if statement you commented on ensures that we also continue looping
> > with whence =3D=3D SEEK_DATA, because that would otherwise prematurely =
end
> > with the new offset =3D -ENXIO.
> >=20
> > >=20
> > > > +	} while (offset =3D=3D end);
> > >=20
> > > 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.
> >=20
> > Wait, lseek() is supposed to return the equivalent of lseek(dm, 0,
> > SEEK_END) when whence =3D=3D SEEK_HOLE and there is data at the end.
>=20
> It was confusing enough for me to write my initial review, I apologize
> if I'm making it harder for you.

No worries, if my code is hard to understand I can learn from your
feedback.

> Yes, we want to ensure that:
>=20
> off1 =3D lseek(fd, -1, SEEK_END);
> off2 =3D off1 + 1; // =3D=3D lseek(fd, 0, SEEK_END)
>=20
> if off1 belongs to a data extent:
>   - lseek(fd, off1, SEEK_DATA) =3D=3D off1
>   - lseek(fd, off1, SEEK_HOLE) =3D=3D off2
>   - lseek(fd, off2, SEEK_DATA) =3D=3D -ENXIO
>   - lseek(fd, off2, SEEK_HOLE) =3D=3D -ENXIO

Agreed.

> if off1 belongs to a hole:
>   - lseek(fd, off1, SEEK_DATA) =3D=3D -ENXIO
>   - lseek(fd, off1, SEEK_HOLE) =3D=3D off1
>   - lseek(fd, off2, SEEK_DATA) =3D=3D -ENXIO
>   - lseek(fd, off2, SEEK_HOLE) =3D=3D -ENXIO

Agreed.

>=20
> Anything in my wall of text from the earlier message inconsistent with
> this table can be ignored; but at the same time, I was not able to
> quickly convince myself that your code properly had those properties,
> even after writing up the table.
>=20
> Reiterating what I said elsewhere, it may be smarter to document that
> for callbacks, it is wiser to require intermediate behavior that the
> input value 'offset' is always between the half-open range
> [ti->begin<<SECTOR_SHIFT, (ti->begin+ti->len)<<SECTOR_SHIFT), and on
> success, the output must be in the fully-closed range [offset,
> (ti->begin+ti->len)<<SECTOR_SHIFT], errors like -EIO are permitted but
> -ENXIO should not be returned; and let the caller worry about
> synthesizing -ENXIO from that (since the caller knows whether or not
> there is a successor ti where adjacency concerns come into play).
>=20
> That is, we can never pass in off2 (beyond the bounds of the table),
> and when passing in off1, I think this interface may be easier to work
> with in the intermediate layers, even though it differs from the
> lseek() interface above.  For off1 in data:
>   - dm_blk_do_seek_hole_data(dm, off1, SEEK_DATA) =3D=3D off1
>   - dm_blk_do_seek_hole_data(dm, off1, SEEK_HOLE) =3D=3D off2
> and for a hole:
>   - dm_blk_do_seek_hole_data(dm, off1, SEEK_DATA) =3D=3D off2
>   - dm_blk_do_seek_hole_data(dm, off1, SEEK_HOLE) =3D=3D off1

I'll take a look again starting from block/fops.c, through dm.c, and
into dm-linear.c to see how to make things clearest. Although I would
like to have the same semantics for every seek function, maybe in the
end your suggestion will make the code clearer. Let's see.

Stefan

--N31yZm+DVUhVXgFt
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmYNmM4ACgkQnKSrs4Gr
c8iAvgf/VrPqldLYrcFp72kYDOeOuDaMtnQx8rd3kAITiBtLHObhWuXOUCa4tb6i
npUgJhyNkUTfzPjkrbvEVjiGsRcVHTPCCAo1OiGU/na+hb2XpU8vAWi7kaoUWco/
K2nyJWOH8WLKSnF7/BysPZDlPa9q2mOsdFIndzIHuNwAQfqTdxvM0Qw6S8TnGmsO
5GAyjMkBQsxssBhymMz9/5HBUdGY1g5mGOJY3d0KB4QZZ3OpnvxcHLynq3RSl+wZ
2dRwccLASS6HiwCeUnWWpFqpe0/EZKkVeGiIlb2iCWK5VMM2eETRXGj/JXYECZfw
+JBARUyqNE2uYBGSpws1RS8qdssnMQ==
=f+tw
-----END PGP SIGNATURE-----

--N31yZm+DVUhVXgFt--