diff mbox

sheepdog: do not blindly memset all read buffers

Message ID 20120627162258.GA13391@lst.de
State New
Headers show

Commit Message

Christoph Hellwig June 27, 2012, 4:22 p.m. UTC 
Only buffers that map to unallocated blocks need to be zeroed.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Comments

MORITA Kazutaka June 28, 2012, 7:06 a.m. UTC | #1 
At Wed, 27 Jun 2012 18:22:58 +0200,
Christoph Hellwig wrote:
> 
> Only buffers that map to unallocated blocks need to be zeroed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: qemu/block/sheepdog.c
> ===================================================================
> --- qemu.orig/block/sheepdog.c	2012-06-27 18:02:41.849867899 +0200
> +++ qemu/block/sheepdog.c	2012-06-27 18:08:35.929865783 +0200
> @@ -1556,18 +1556,24 @@ static int coroutine_fn sd_co_rw_vector(
>  
>          len = MIN(total - done, SD_DATA_OBJ_SIZE - offset);
>  
> -        if (!inode->data_vdi_id[idx]) {
> -            if (acb->aiocb_type == AIOCB_READ_UDATA) {
> +
> +        switch (acb->aiocb_type) {
> +        case AIOCB_READ_UDATA:
> +            if (!inode->data_vdi_id[idx]) {
> +                qemu_iovec_memset_skip(acb->qiov, 0, len, offset);

'offset' is the offset of the sheepdog object.  I think it should be
'done' since we need to pass the number of skip bytes.

>                  goto done;
>              }
> -
> -            create = 1;
> -        } else if (acb->aiocb_type == AIOCB_WRITE_UDATA
> -                   && !is_data_obj_writable(inode, idx)) {
> -            /* Copy-On-Write */
> -            create = 1;
> -            old_oid = oid;
> -            flags = SD_FLAG_CMD_COW;
> +            break;
> +         case AIOCB_WRITE_UDATA:

Wrong indentation.

Thanks,

Kazutaka
Christoph Hellwig June 29, 2012, 3:38 p.m. UTC | #2 
On Thu, Jun 28, 2012 at 04:06:24PM +0900, MORITA Kazutaka wrote:
> 
> 'offset' is the offset of the sheepdog object.  I think it should be
> 'done' since we need to pass the number of skip bytes.

Indeed.  Odd that mny tests didn't catch this.

> 
> >                  goto done;
> >              }
> > -
> > -            create = 1;
> > -        } else if (acb->aiocb_type == AIOCB_WRITE_UDATA
> > -                   && !is_data_obj_writable(inode, idx)) {
> > -            /* Copy-On-Write */
> > -            create = 1;
> > -            old_oid = oid;
> > -            flags = SD_FLAG_CMD_COW;
> > +            break;
> > +         case AIOCB_WRITE_UDATA:
> 
> Wrong indentation.

Where?  At least I can't find anything obvious and checkpath.pl is fine
with the patch, too.
MORITA Kazutaka July 2, 2012, 9:33 a.m. UTC | #3 
At Fri, 29 Jun 2012 17:38:24 +0200,
Christoph Hellwig wrote:
> > 
> > >                  goto done;
> > >              }
> > > -
> > > -            create = 1;
> > > -        } else if (acb->aiocb_type == AIOCB_WRITE_UDATA
> > > -                   && !is_data_obj_writable(inode, idx)) {
> > > -            /* Copy-On-Write */
> > > -            create = 1;
> > > -            old_oid = oid;
> > > -            flags = SD_FLAG_CMD_COW;
> > > +            break;
> > > +         case AIOCB_WRITE_UDATA:
> > 
> > Wrong indentation.
> 
> Where?  At least I can't find anything obvious and checkpath.pl is fine
> with the patch, too.

It seems that there is a redundant space before "case AIOCB_WRITE_UDATA:".

==
$ ./scripts/checkpatch.pl your.patch
ERROR: switch and case should be at the same indent
#92: FILE: block/sheepdog.c:1560:
+        switch (acb->aiocb_type) {
[...]
+         case AIOCB_WRITE_UDATA:

total: 1 errors, 0 warnings, 55 lines checked

c.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
diff mbox

Patch

Index: qemu/block/sheepdog.c
===================================================================
--- qemu.orig/block/sheepdog.c	2012-06-27 18:02:41.849867899 +0200
+++ qemu/block/sheepdog.c	2012-06-27 18:08:35.929865783 +0200
@@ -1556,18 +1556,24 @@  static int coroutine_fn sd_co_rw_vector(
 
         len = MIN(total - done, SD_DATA_OBJ_SIZE - offset);
 
-        if (!inode->data_vdi_id[idx]) {
-            if (acb->aiocb_type == AIOCB_READ_UDATA) {
+
+        switch (acb->aiocb_type) {
+        case AIOCB_READ_UDATA:
+            if (!inode->data_vdi_id[idx]) {
+                qemu_iovec_memset_skip(acb->qiov, 0, len, offset);
                 goto done;
             }
-
-            create = 1;
-        } else if (acb->aiocb_type == AIOCB_WRITE_UDATA
-                   && !is_data_obj_writable(inode, idx)) {
-            /* Copy-On-Write */
-            create = 1;
-            old_oid = oid;
-            flags = SD_FLAG_CMD_COW;
+            break;
+         case AIOCB_WRITE_UDATA:
+            if (!inode->data_vdi_id[idx]) {
+                create = 1;
+            } else if (!is_data_obj_writable(inode, idx)) {
+                /* Copy-On-Write */
+                create = 1;
+                old_oid = oid;
+                flags = SD_FLAG_CMD_COW;
+            }
+            break;
         }
 
         if (create) {
@@ -1655,20 +1661,12 @@  static coroutine_fn int sd_co_readv(Bloc
                        int nb_sectors, QEMUIOVector *qiov)
 {
     SheepdogAIOCB *acb;
-    int i, ret;
+    int ret;
 
     acb = sd_aio_setup(bs, qiov, sector_num, nb_sectors, NULL, NULL);
     acb->aiocb_type = AIOCB_READ_UDATA;
     acb->aio_done_func = sd_finish_aiocb;
 
-    /*
-     * TODO: we can do better; we don't need to initialize
-     * blindly.
-     */
-    for (i = 0; i < qiov->niov; i++) {
-        memset(qiov->iov[i].iov_base, 0, qiov->iov[i].iov_len);
-    }
-
     ret = sd_co_rw_vector(acb);
     if (ret <= 0) {
         qemu_aio_release(acb);