Received-SPF: pass (google.com: domain of linux-kernel+bounces-129946-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 3 Apr 2024 10:22:16 -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 6/9] dm-linear: add llseek(SEEK_HOLE/SEEK_DATA) support
Message-ID: <20240403142216.GF2524049@fedora>
References: <20240328203910.2370087-1-stefanha@redhat.com>
 <20240328203910.2370087-7-stefanha@redhat.com>
 <zetfekdpoq6rmas26o7jl2uvricjcv6zygi6cngf6mkmiev5kn@e5d4ie3m77ku>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="ThZrXlOWPAzKv2kI"
Content-Disposition: inline
In-Reply-To: <zetfekdpoq6rmas26o7jl2uvricjcv6zygi6cngf6mkmiev5kn@e5d4ie3m77ku>


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

On Thu, Mar 28, 2024 at 07:54:21PM -0500, Eric Blake wrote:
> On Thu, Mar 28, 2024 at 04:39:07PM -0400, Stefan Hajnoczi wrote:
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  drivers/md/dm-linear.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >=20
> > diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
> > index 2d3e186ca87e3..9b6cdfa4f951d 100644
> > --- a/drivers/md/dm-linear.c
> > +++ b/drivers/md/dm-linear.c
> > @@ -147,6 +147,30 @@ static int linear_report_zones(struct dm_target *t=
i,
> >  #define linear_report_zones NULL
> >  #endif
> > =20
> > +static loff_t linear_seek_hole_data(struct dm_target *ti, loff_t offse=
t,
> > +		int whence)
> > +{
> > +	struct linear_c *lc =3D ti->private;
> > +	loff_t ti_begin =3D ti->begin << SECTOR_SHIFT;
> > +	loff_t ti_len =3D ti->len << SECTOR_SHIFT;
> > +	loff_t bdev_start =3D lc->start << SECTOR_SHIFT;
> > +	loff_t bdev_offset;
>=20
> Okay, given my questions in 4/9, it looks like your intent is that
> each callback for dm_seek_hole_data will obey its own ti-> limits.

Yes, exactly.

>=20
> > +
> > +	/* TODO underflow/overflow? */
> > +	bdev_offset =3D offset - ti_begin + bdev_start;
> > +
> > +	bdev_offset =3D blkdev_seek_hole_data(lc->dev->bdev, bdev_offset,
> > +					    whence);
> > +	if (bdev_offset < 0)
> > +		return bdev_offset;
> > +
> > +	offset =3D bdev_offset - bdev_start;
> > +	if (offset >=3D ti_len)
> > +		return whence =3D=3D SEEK_DATA ? -ENXIO : ti_begin + ti_len;
>=20
> However, this is inconsistent with dm_blk_seek_hole_data_default in
> 4/9; I think you want to unconditionally return -ENXIO here, and let
> the caller figure out when to turn -ENXIO back into end to proceed
> with the next ti in the list.
>=20
> OR, you may want to document the semantics that dm_seek_hole_data
> callbacks must NOT return -ENXIO, but always return ti_begin + ti_len
> when the answer (either SEEK_HOLE or SEEK_END) did not lie within the
> current ti - it is DIFFERENT than the semantics for
> blkdev_seek_hole_data, but gets normalized back into the expected
> -ENXIO answer when dm_blk_do_seek_hole_data finally advances past the
> last ti.
>=20
> At any rate, I know this is an RFC series, but it goes to show that
> comments will be essential, whichever interface you decide for
> callbacks to honor (both a guarantee that callbacks will only ever see
> SEEK_HOLE/SEEK_DATA in bounds, because earlier points in the call
> stack have filtered out out-of-bounds and SEEK_SET; and constraints on
> what the return value(s) must be for the various callbacks, especially
> if it is different from the eventual return value of the overall
> llseek syscall)

It's easier to understand the code when lseek function has the same
semantics, so I'd rather not customize the semantics for certain lseek
functions.

I'll make sure that the device-mapper targets match the
dm_blk_seek_hole_data_default() behavior. To be honest, I relied on dm.c
always passing offset values that are within the target, but that in
itself is customizing the semantics :).

--ThZrXlOWPAzKv2kI
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmYNZhgACgkQnKSrs4Gr
c8hVBwgAl+Oz9WpdlpjFEbKoWMaJLh87SOHRM4LmyIy6J2xn736XJygEI434ZFda
OJrd1q8tc7E2r97Qg6m48sFXDJKsRwtqS/3g+sLaSKbtTgHkZEOQMHLjazDMtZC0
YPDyuzXMyupUDGNGkgQmAan1CXjIM0nVZekB/aIEBuAqOjXgPS02HAOgwM83o7O/
m55VskGkdJfM9WLg6yW32Y8HPstVkM2kK+67p7bSfXuGRwfkFj9v5vLqG93LVRLG
FWyh1qeBW2ufk1/ygBXasP3Fe60DlK36V2nrRs2tTOWXOMsNQUsq/fygjHlZmByI
n7SCc62GB/dCK5e6X7kHgpOyjd8AfA==
=sXQ1
-----END PGP SIGNATURE-----

--ThZrXlOWPAzKv2kI--