Received-SPF: pass (google.com: domain of linux-kernel+bounces-123848-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 28 Mar 2024 19:00:26 -0500
From: Eric Blake <eblake@redhat.com>
To: Stefan Hajnoczi <stefanha@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: <wvdjesmnq6xrkanncathyciocbtxa6m3fefvx3za3ikxfs7uqx@wo22n4cvndr3>
References: <20240328203910.2370087-1-stefanha@redhat.com>
 <20240328203910.2370087-3-stefanha@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240328203910.2370087-3-stefanha@redhat.com>
User-Agent: NeoMutt/20240201

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(-)
> 
> 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);
>  }
>  
> +static loff_t lo_seek_hole_data(struct block_device *bdev, loff_t offset,
> +		int whence)
> +{
> +	/* TODO need to activate cgroups or use worker? */
> +	/* TODO locking? */
> +	struct loop_device *lo = bdev->bd_disk->private_data;
> +	struct file *file = lo->lo_backing_file;
> +
> +	if (lo->lo_offset > 0)
> +		offset += lo->lo_offset; /* TODO underflow/overflow? */
> +
> +	/* TODO backing file offset is modified! */
> +	offset = vfs_llseek(file, offset, whence);

Not only did you modify the underlying offset...

> +	if (offset < 0)
> +		return offset;
> +
> +	if (lo->lo_offset > 0)
> +		offset -= lo->lo_offset; /* TODO underflow/overflow? */
> +	if (lo->lo_sizelimit > 0 && offset > lo->lo_sizelimit)
> +		offset = lo->lo_sizelimit;

..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 =
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).

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

But for an RFC, the idea of being able to seek to HOLEs in a loop
device is awesome!

> @@ -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;
>  	}
> -		
> +
>  	/* Hide this loop device for serialization. */
>  	ret = mutex_lock_killable(&loop_ctl_mutex);
>  	if (ret)

Unrelated whitespace change?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org