[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v8 08/21] iscsi: Switch to .bdrv_co_block_status()
Date: Wed, 14 Feb 2018 08:33:30 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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.


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;
+    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) {
-    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {
          *file = bs;

Can file ever be NULL?


      return ret;
@@ -800,25 +798,24 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState 
                                   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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]