[PATCH ubdsrv] tgt_null: Return number of sectors read/written

[PATCH ubdsrv] tgt_null: Return number of sectors read/written

[Gabriel Krisman Bertazi]

Hi Ming,

I wrote this against your devel-v3 branch.  I'm wondering if you plan to
send a new version of the kernel patch soon? From the latest
discussions, I don't think there were major issues found on review. :)

I hope people don't mind I cc'd linux-block about this userspace code.
Please, let me know if I shouldn't do that.

-- >8 --

The number of sectors read/written is used to verify forward progress of
the request inside the kernel.  If we return 0 here, the kernel
understands that as an IO failure (see first check in ubd_complete_rq),
and will reissue the request, causing an infinite loop of unfullfilled
requests.  This can be reproduced with:

  ubdsrv/ubd add -t null -n0 -q1 -d1
  dd if=/dev/vda of=/dev/ubdb0 count=1 bs=4k

The approach minics nullblk, which returns the total IO size.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 tgt_null.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tgt_null.c b/tgt_null.c
index 85636c405f0c..61850a2cd046 100644
--- a/tgt_null.c
+++ b/tgt_null.c
@@ -20,7 +20,9 @@ static int null_init_tgt(struct ubdsrv_tgt_info *tgt, int type, int argc,
 static int null_handle_io_async(struct ubdsrv_queue *q, struct ubd_io *io,
 		int tag)
 {
-	ubdsrv_mark_io_done(io, 0);
+	const struct ubdsrv_io_desc *iod = ubdsrv_get_iod(q, tag);
+
+	ubdsrv_mark_io_done(io, iod->nr_sectors << 9);
 
 	return 0;
 }
-- 
2.36.1

Re: [PATCH ubdsrv] tgt_null: Return number of sectors read/written

[Ming Lei]

On Tue, Jun 21, 2022 at 06:48:39PM -0400, Gabriel Krisman Bertazi wrote:
> Hi Ming,
> 
> I wrote this against your devel-v3 branch.  I'm wondering if you plan to
> send a new version of the kernel patch soon? From the latest

Yeah, that is on my todo list:

https://github.com/ming1/linux/commits/my_for-5.19-ubd-devel_v3

there has lots cleanup & improvement.

> discussions, I don't think there were major issues found on review. :)

One problem is the driver name, and Christoph thought we have
'arch/um/drivers/ubd*.c'. Not thought of one good candidate yet.

> 
> I hope people don't mind I cc'd linux-block about this userspace code.
> Please, let me know if I shouldn't do that.
> 
> -- >8 --
> 
> The number of sectors read/written is used to verify forward progress of
> the request inside the kernel.  If we return 0 here, the kernel
> understands that as an IO failure (see first check in ubd_complete_rq),
> and will reissue the request, causing an infinite loop of unfullfilled
> requests.  This can be reproduced with:
> 
>   ubdsrv/ubd add -t null -n0 -q1 -d1
>   dd if=/dev/vda of=/dev/ubdb0 count=1 bs=4k
> 
> The approach minics nullblk, which returns the total IO size.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  tgt_null.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tgt_null.c b/tgt_null.c
> index 85636c405f0c..61850a2cd046 100644
> --- a/tgt_null.c
> +++ b/tgt_null.c
> @@ -20,7 +20,9 @@ static int null_init_tgt(struct ubdsrv_tgt_info *tgt, int type, int argc,
>  static int null_handle_io_async(struct ubdsrv_queue *q, struct ubd_io *io,
>  		int tag)
>  {
> -	ubdsrv_mark_io_done(io, 0);
> +	const struct ubdsrv_io_desc *iod = ubdsrv_get_iod(q, tag);
> +
> +	ubdsrv_mark_io_done(io, iod->nr_sectors << 9);

This issue was actually fixed in master branch, and there are actually lots
of change in master:

- switch to liburing
- switch to c++, and use c++20 coroutine for ->handle_io_async()
- all kinds of cleanup

I am actually working on ubd-qcow2 with above, but that may take a while and
the code isn't posted yet. The motivation is that the whole framework can
get verified well enough with one complicated target implementation.

Thanks,
Ming

Re: [PATCH ubdsrv] tgt_null: Return number of sectors read/written

[Gabriel Krisman Bertazi]

Ming Lei <ming.lei@redhat.com> writes:

> On Tue, Jun 21, 2022 at 06:48:39PM -0400, Gabriel Krisman Bertazi wrote:
>> Hi Ming,
>> 
>> I wrote this against your devel-v3 branch.  I'm wondering if you plan to
>> send a new version of the kernel patch soon? From the latest
>
> Yeah, that is on my todo list:
>
> https://github.com/ming1/linux/commits/my_for-5.19-ubd-devel_v3
>
> there has lots cleanup & improvement.
>
>> discussions, I don't think there were major issues found on review. :)
>
> One problem is the driver name, and Christoph thought we have
> 'arch/um/drivers/ubd*.c'. Not thought of one good candidate yet.

Hi Ming,

Thanks for the info, and sorry for not noticing the fix merged on Jun, 3rd
on the master branch.  I will follow that branch when testing and submit
fixes I find along the way.

I guess you have considered a lot of names, but I'd suggest any of:

 * blkuser,
 * ublk
 * BUSE (as in Block FUSE, though there is another non-upstream
project with that name),
 * UBIO (as in UIO, but for Block IO)
 * B2U (Block IO Backed by userspace) :-P

TBH, my favorite is ublk.

Thank you,

-- 
Gabriel Krisman Bertazi

Re: [PATCH ubdsrv] tgt_null: Return number of sectors read/written

[Ming Lei]

On Wed, Jun 22, 2022 at 12:23:42PM -0400, Gabriel Krisman Bertazi wrote:
> Ming Lei <ming.lei@redhat.com> writes:
> 
> > On Tue, Jun 21, 2022 at 06:48:39PM -0400, Gabriel Krisman Bertazi wrote:
> >> Hi Ming,
> >> 
> >> I wrote this against your devel-v3 branch.  I'm wondering if you plan to
> >> send a new version of the kernel patch soon? From the latest
> >
> > Yeah, that is on my todo list:
> >
> > https://github.com/ming1/linux/commits/my_for-5.19-ubd-devel_v3
> >
> > there has lots cleanup & improvement.
> >
> >> discussions, I don't think there were major issues found on review. :)
> >
> > One problem is the driver name, and Christoph thought we have
> > 'arch/um/drivers/ubd*.c'. Not thought of one good candidate yet.
> 
> Hi Ming,
> 
> Thanks for the info, and sorry for not noticing the fix merged on Jun, 3rd
> on the master branch.  I will follow that branch when testing and submit
> fixes I find along the way.

That is great!

> 
> I guess you have considered a lot of names, but I'd suggest any of:
> 
>  * blkuser,
>  * ublk
>  * BUSE (as in Block FUSE, though there is another non-upstream
> project with that name),
>  * UBIO (as in UIO, but for Block IO)
>  * B2U (Block IO Backed by userspace) :-P
> 
> TBH, my favorite is ublk.

Me too, will change to it in v3 if no one objects.

Thanks,
Ming