Re: [Qemu-block] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()

[Eric Blake]

On 02/14/2018 05:53 AM, Kevin Wolf wrote:
> Am 13.02.2018 um 21:26 hat Eric Blake geschrieben:
> > We are gradually moving away from sector-based interfaces, towards
> > byte-based.  Update the iscsi driver accordingly.  In this case,
> > it is handy to teach iscsi_co_block_status() to handle a NULL map
> > and file parameter, even though the block layer passes non-NULL
> > values, because we also call the function directly.  For now, there
> > are no optimizations done based on the want_zero flag.

[1]

> > We can also make the simplification of asserting that the block
> > layer passed in aligned values.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> > Reviewed-by: Fam Zheng <address@hidden>

> >       /* default to all sectors allocated */
> > -    ret = BDRV_BLOCK_DATA;
> > -    ret |= (sector_num << BDRV_SECTOR_BITS) | BDRV_BLOCK_OFFSET_VALID;
> > -    *pnum = nb_sectors;
> > +    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> > +    if (map) {
> > +        *map = offset;
> > +    }
>
> Can map ever be NULL? You didn't have that check in other drivers.

The documentation in block_int.h states that io.c never passes NULL for 
map.  However, see my commit message [1], and the code below [2], for 
why THIS driver has to check for NULL.


> > @@ -760,7 +758,7 @@ out:
> >       if (iTask.task != NULL) {
> >           scsi_free_scsi_task(iTask.task);
> >       }
> > -    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
> > +    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
> >           *file = bs;
> >       }
>
> Can file ever be NULL?

Ditto.

> >       return ret;
> > @@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState 
> > *bs,
> >                                    nb_sectors * BDRV_SECTOR_SIZE) &&
>
> No iscsi_co_preadv() yet... :-(

Yeah, but that's for another day.

> >           !iscsi_allocmap_is_allocated(iscsilun, sector_num * BDRV_SECTOR_SIZE,
> >                                        nb_sectors * BDRV_SECTOR_SIZE)) {
> > -        int pnum;
> > -        BlockDriverState *file;
> > +        int64_t pnum;
> >           /* check the block status from the beginning of the cluster
> >            * containing the start sector */
> > -        int cluster_sectors = iscsilun->cluster_size >> BDRV_SECTOR_BITS;
> > -        int head;
> > -        int64_t ret;
> > +        int64_t head;
> > +        int ret;
> >
> > -        assert(cluster_sectors);
> > -        head = sector_num % cluster_sectors;
> > -        ret = iscsi_co_get_block_status(bs, sector_num - head,
> > -                                        BDRV_REQUEST_MAX_SECTORS, &pnum,
> > -                                        &file);
> > +        assert(iscsilun->cluster_size);
> > +        head = (sector_num * BDRV_SECTOR_SIZE) % iscsilun->cluster_size;
> > +        ret = iscsi_co_block_status(bs, false,
> > +                                    sector_num * BDRV_SECTOR_SIZE - head,
> > +                                    BDRV_REQUEST_MAX_BYTES, &pnum, NULL, NULL);

[2] This is the reason that THIS driver has to check for NULL, even 
though the block layer never passes NULL.

> It doesn't make a difference with your current implementation because it
> ignores want_zero, but consistent with your approach that
> want_zero=false returns just that everyhting is allocated for drivers
> without support for backing files, I think you want want_zero=true here.

Makes sense.  If that's the only tweak, can you make it while taking the 
series, or will I need to respin?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org