Received-SPF: pass (google.com: domain of linux-kernel+bounces-125046-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 29 Mar 2024 08:54:05 -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 2/9] loop: add llseek(SEEK_HOLE/SEEK_DATA) support
Message-ID: <20240329125405.GA2382288@fedora>
References: <20240328203910.2370087-1-stefanha@redhat.com>
 <20240328203910.2370087-3-stefanha@redhat.com>
 <wvdjesmnq6xrkanncathyciocbtxa6m3fefvx3za3ikxfs7uqx@wo22n4cvndr3>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="O0O0CfUZc75I8i/Q"
Content-Disposition: inline
In-Reply-To: <wvdjesmnq6xrkanncathyciocbtxa6m3fefvx3za3ikxfs7uqx@wo22n4cvndr3>


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

On Thu, Mar 28, 2024 at 07:00:26PM -0500, Eric Blake wrote:
> On Thu, Mar 28, 2024 at 04:39:03PM -0400, Stefan Hajnoczi wrote:
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > Open issues:
> > - The file offset is updated on both the blkdev file and the backing
> >   file. Is there a way to avoid updating the backing file offset so the
> >   file opened by userspace is not affected?
> > - Should this run in the worker or use the cgroups?
> > ---
> >  drivers/block/loop.c | 36 ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >=20
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 28a95fd366fea..6a89375de82e8 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -750,6 +750,29 @@ static void loop_sysfs_exit(struct loop_device *lo)
> >  				   &loop_attribute_group);
> >  }
> > =20
> > +static loff_t lo_seek_hole_data(struct block_device *bdev, loff_t offs=
et,
> > +		int whence)
> > +{
> > +	/* TODO need to activate cgroups or use worker? */
> > +	/* TODO locking? */
> > +	struct loop_device *lo =3D bdev->bd_disk->private_data;
> > +	struct file *file =3D lo->lo_backing_file;
> > +
> > +	if (lo->lo_offset > 0)
> > +		offset +=3D lo->lo_offset; /* TODO underflow/overflow? */
> > +
> > +	/* TODO backing file offset is modified! */
> > +	offset =3D vfs_llseek(file, offset, whence);
>=20
> Not only did you modify the underlying offset...
>=20
> > +	if (offset < 0)
> > +		return offset;
> > +
> > +	if (lo->lo_offset > 0)
> > +		offset -=3D lo->lo_offset; /* TODO underflow/overflow? */
> > +	if (lo->lo_sizelimit > 0 && offset > lo->lo_sizelimit)
> > +		offset =3D lo->lo_sizelimit;
>=20
> ...but if this code kicks in, you have clamped the return result to
> EOF of the loop device while leaving the underlying offset beyond the
> limit, which may mess up assumptions of other code expecting the loop
> to always have an in-bounds offset for the underlying file (offhand, I
> don't know if there is any such code; but since loop_ctl_fops.llseek =3D
> noop_lseek, there may be code relying on an even-tighter restriction
> that the offset of the underlying file never changes, not even within
> bounds).

I don't think anything relies on the file offset. Requests coming from
the block device contain their own offset (which may have been based on
the block device file's offset, but not the backing file's offset).

> Furthermore, this is inconsistent with all other seek-beyond-end code
> that fails with -ENXIO instead of returning size.

You're right, in the SEEK_DATA case the return value should be -ENXIO.
The SEEK_HOLE case is correct with lo_sizelimit. There is also an
off-by-one error in the comparison.

It should be:

  if (lo->lo_sizelimit > 0 && offset >=3D lo->lo_sizelimit) {
  	if (whence =3D=3D SEEK_DATA)
  		offset =3D -ENXIO;
  	else
  		offset =3D lo->lo_sizelimit;
  }

> But for an RFC, the idea of being able to seek to HOLEs in a loop
> device is awesome!
>=20
> > @@ -2140,7 +2164,7 @@ static int loop_control_remove(int idx)
> >  		pr_warn_once("deleting an unspecified loop device is not supported.\=
n");
> >  		return -EINVAL;
> >  	}
> > -	=09
> > +
> >  	/* Hide this loop device for serialization. */
> >  	ret =3D mutex_lock_killable(&loop_ctl_mutex);
> >  	if (ret)
>=20
> Unrelated whitespace change?

Yes, I'll drop it.

Stefan

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

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

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmYGue0ACgkQnKSrs4Gr
c8jpDggAtKqqz4GIKlteqyYksQXgYleVmkngDOnQAQp9bkmubJmmml9ZX6CMhB7X
HnmKX80aoDVIcy5KTFh1bcRLGJgzidr5vjuO3UT5MP3P+fEMiZzp2EfBRGqUsnY9
Sooj/8pCs7vLKcfXxKLRH15ME4+1tj1LhG8LVZRhKLYtuHXPE9MP3N4x4lwPOWy5
vypvEnduzoCor/mPGM7OUMroQd7BZtuZteACWSoI77koQ3pB/296Z5WJv3+Sg/SJ
jdJX963d2ylggR3CMeetgftzGfpP5Cb/ev26LwbTpAyLe3Uv2hBobhQaB2pKZ9ry
17rpr2bJwObaeAqsXmVDPAlUOqAHTg==
=iCZg
-----END PGP SIGNATURE-----

--O0O0CfUZc75I8i/Q--