Re: [Qemu-devel] [PATCH 6/9] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

[Badari Pulavarty]

On 05/29/2013 10:36 PM, Nicholas A. Bellinger wrote:
> On Wed, 2013-05-29 at 21:29 -0700, Nicholas A. Bellinger wrote:
> > On Thu, 2013-05-30 at 06:17 +0800, Asias He wrote:
> > > On Wed, May 29, 2013 at 08:10:44AM -0700, Badari Pulavarty wrote:
> > > > On 05/29/2013 02:05 AM, Wenchao Xia wrote:
> > > > > 于 2013-5-28 17:00, Wenchao Xia 写道:
> > <SNIP>
> >
> > > > >   I have done a basic test of vhost-scsi, following is the result I'd
> > > > > like to post, generally it seems fine:
> > > > >
> > > > > Result:
> > > > >   fdisk/mkfs: fdisk can find it, mke2fs works fine.
> > > > >   mount: can mount it.
> > > > >   file I/O: dd 90M zero to a file in that disk succeed.
> > > >
> > > >
> > > > I tried without nested kvm.
> > > >
> > > > > Issues:
> > > > >   1) in fdisk -l, sometime timeout with dmesg "end_request: I/O error,
> > > > > dev  fd0, sector 0", I guess it is caused by nested KVM that failed
> > > > > to kick host kernel?
> > > >
> > > > I don't see this issue. Are you sure "fd0" is actually the scsi device ?
> > > > what is "fd0" ?
> > > >
> > > > >   2) in fdisk -l, it shows 512 bytes larger than the parameter I
> > > > > specified in fd_dev_size parameter in configfs on host.(shows
> > > > > 104858112 bytes, see the invocation script below)
> > > > I see the same. For some reason "fdisk -l" in the VM shows
> > > > 512-bytes more than the actual size for the file (on the host).
> > > Hmm, interesting. Will look into it.
> > >
> > > Nick, Any ideas here?
> > Mmm, fd_get_blocks() is not returning the expected minus one logical
> > blocks with a !S_ISBLK() setup.
> >
> > This is happening for every other backend ->get_blocks() call already,
> > and should be happening for the fd_dev_size case as well.
> >
> > Applying the following to target-pending.git now.
> Actually sorry, that last patch is not correct..
>
> Here's a better one to properly set fd_dev->fd_block_size at configure
> time, and use dev_attrib.block_size in fd_get_blocks() to allow for user
> defined block_sizes.
>
> Thanks,
>
> --nab
>
> commit 9e309f9307fe644dee8718980bfcb77de91ce38e
> Author: Nicholas Bellinger <address@hidden>
> Date:   Wed May 29 21:35:23 2013 -0700
>
>      target/file: Fix off-by-one READ_CAPACITY bug for !S_ISBLK export
>      
>      This patch fixes a bug where FILEIO was incorrectly reporting the number
>      of logical blocks (+ 1) when using non struct block_device export mode.
>      
>      It changes fd_get_blocks() to follow all other backend ->get_blocks() cases,
>      and reduces the calculated dev_size by one dev->dev_attrib.block_size
>      number of bytes, and fixes the initial block_size assignment within
>      fd_configure_device()
>      
>      Reported-by: Wenchao Xia <address@hidden>
>      Reported-by: Badari Pulavarty <address@hidden>
>      Signed-off-by: Nicholas Bellinger <address@hidden>
>
> diff --git a/drivers/target/target_core_file.c 
> b/drivers/target/target_core_file.c
> index 1b1d544..b11890d 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -153,6 +153,7 @@ static int fd_configure_device(struct se_device *dev)
>                  struct request_queue *q = bdev_get_queue(inode->i_bdev);
>                  unsigned long long dev_size;
>
> +               fd_dev->fd_block_size = bdev_logical_block_size(inode->i_bdev);
>                  /*
>                   * Determine the number of bytes from i_size_read() minus
>                   * one (1) logical sector from underlying struct block_device
> @@ -199,6 +200,7 @@ static int fd_configure_device(struct se_device *dev)
>                          goto fail;
>                  }
>
> +               fd_dev->fd_block_size = FD_BLOCKSIZE;
>                  /*
>                   * Limit UNMAP emulation to 8k Number of LBAs (NoLB)
>                   */
> @@ -217,9 +219,7 @@ static int fd_configure_device(struct se_device *dev)
>                  dev->dev_attrib.max_write_same_len = 0x1000;
>          }
>
> -       fd_dev->fd_block_size = dev->dev_attrib.hw_block_size;
> -
> -       dev->dev_attrib.hw_block_size = FD_BLOCKSIZE;
> +       dev->dev_attrib.hw_block_size = fd_dev->fd_block_size;
>          dev->dev_attrib.hw_max_sectors = FD_MAX_SECTORS;
>          dev->dev_attrib.hw_queue_depth = FD_MAX_DEVICE_QUEUE_DEPTH;
>
> @@ -694,11 +694,12 @@ static sector_t fd_get_blocks(struct se_device *dev)
>           * to handle underlying block_device resize operations.
>           */
>          if (S_ISBLK(i->i_mode))
> -               dev_size = (i_size_read(i) - fd_dev->fd_block_size);
> +               dev_size = i_size_read(i);
>          else
>                  dev_size = fd_dev->fd_dev_size;
>
> -       return div_u64(dev_size, dev->dev_attrib.block_size);
> +       return div_u64(dev_size - dev->dev_attrib.block_size,
> +                      dev->dev_attrib.block_size);
>   }
>
>   static struct sbc_ops fd_sbc_ops = {
Verified and it shows the correct value now.

Thanks,
Badari