xen_disk: remove syncwrite option

Submitted by Stefano Stabellini on April 24, 2012, 11:22 a.m.

Details

Message ID 1335266521-20875-1-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini April 24, 2012, 11:22 a.m.
Use the BDRV_O_CACHE_* flags instead.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_disk.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

Comments

Kevin Wolf April 24, 2012, 11:37 a.m.
Am 24.04.2012 13:22, schrieb Stefano Stabellini:
> Use the BDRV_O_CACHE_* flags instead.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Doesn't apply to qemu.git because...

> ---
>  hw/xen_disk.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 4a6d89c..3e4a47b 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -48,7 +48,6 @@
>  
>  /* ------------------------------------------------------------- */
>  
> -static int syncwrite    = 0;
>  static int batch_maps   = 0;
>  
>  static int max_requests = 32;
> @@ -189,15 +188,10 @@ static int ioreq_parse(struct ioreq *ioreq)
>              ioreq->presync = 1;
>              return 0;
>          }
> -        if (!syncwrite) {
> -            ioreq->postsync = 1;

...this is ioreq->presync = ioreq->postsync = 1;

And while we're at it, the commit message could mention that there was
no way to set this flag anyway, so we're just removing dead code.

Kevin
Stefano Stabellini April 24, 2012, 11:55 a.m.
On Tue, 24 Apr 2012, Kevin Wolf wrote:
> Am 24.04.2012 13:22, schrieb Stefano Stabellini:
> > Use the BDRV_O_CACHE_* flags instead.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Doesn't apply to qemu.git because...
> 
> > ---
> >  hw/xen_disk.c |    8 +-------
> >  1 files changed, 1 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> > index 4a6d89c..3e4a47b 100644
> > --- a/hw/xen_disk.c
> > +++ b/hw/xen_disk.c
> > @@ -48,7 +48,6 @@
> >  
> >  /* ------------------------------------------------------------- */
> >  
> > -static int syncwrite    = 0;
> >  static int batch_maps   = 0;
> >  
> >  static int max_requests = 32;
> > @@ -189,15 +188,10 @@ static int ioreq_parse(struct ioreq *ioreq)
> >              ioreq->presync = 1;
> >              return 0;
> >          }
> > -        if (!syncwrite) {
> > -            ioreq->postsync = 1;
> 
> ...this is ioreq->presync = ioreq->postsync = 1;

That's because it is based on top of the following two patches:

xen_disk: use bdrv_aio_flush instead of bdrv_flush
http://marc.info/?l=qemu-devel&m=133434258008959&w=2

xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER
http://marc.info/?l=qemu-devel&m=133459581417878&w=2

I have just sent a pull request for them
(http://marc.info/?l=qemu-devel&m=133526669019403) after leaving lying
around for a while.
Next time do you want me to send xen_disk specific pull request to you
instead? Sorry if I shouldn't have sent the pull request directly...


> And while we're at it, the commit message could mention that there was
> no way to set this flag anyway, so we're just removing dead code.

good point
Kevin Wolf April 24, 2012, noon
Am 24.04.2012 13:55, schrieb Stefano Stabellini:
> On Tue, 24 Apr 2012, Kevin Wolf wrote:
>> Am 24.04.2012 13:22, schrieb Stefano Stabellini:
>>> Use the BDRV_O_CACHE_* flags instead.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>> Doesn't apply to qemu.git because...
>>
>>> ---
>>>  hw/xen_disk.c |    8 +-------
>>>  1 files changed, 1 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
>>> index 4a6d89c..3e4a47b 100644
>>> --- a/hw/xen_disk.c
>>> +++ b/hw/xen_disk.c
>>> @@ -48,7 +48,6 @@
>>>  
>>>  /* ------------------------------------------------------------- */
>>>  
>>> -static int syncwrite    = 0;
>>>  static int batch_maps   = 0;
>>>  
>>>  static int max_requests = 32;
>>> @@ -189,15 +188,10 @@ static int ioreq_parse(struct ioreq *ioreq)
>>>              ioreq->presync = 1;
>>>              return 0;
>>>          }
>>> -        if (!syncwrite) {
>>> -            ioreq->postsync = 1;
>>
>> ...this is ioreq->presync = ioreq->postsync = 1;
> 
> That's because it is based on top of the following two patches:
> 
> xen_disk: use bdrv_aio_flush instead of bdrv_flush
> http://marc.info/?l=qemu-devel&m=133434258008959&w=2
> 
> xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER
> http://marc.info/?l=qemu-devel&m=133459581417878&w=2
> 
> I have just sent a pull request for them
> (http://marc.info/?l=qemu-devel&m=133526669019403) after leaving lying
> around for a while.
> Next time do you want me to send xen_disk specific pull request to you
> instead? Sorry if I shouldn't have sent the pull request directly...

I see. I wasn't aware that you're doing pull requests, but it isn't a
problem. It just means that you should probably do a pull request for
this one as well instead of expecting that I pick it up. But you can
have my Acked-by, if you like.

Should I ignore xen_disk patches from now on for the block branch and
assume that you pick them up?

Kevin
Stefano Stabellini April 24, 2012, 12:43 p.m.
On Tue, 24 Apr 2012, Kevin Wolf wrote:
> > That's because it is based on top of the following two patches:
> > 
> > xen_disk: use bdrv_aio_flush instead of bdrv_flush
> > http://marc.info/?l=qemu-devel&m=133434258008959&w=2
> > 
> > xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER
> > http://marc.info/?l=qemu-devel&m=133459581417878&w=2
> > 
> > I have just sent a pull request for them
> > (http://marc.info/?l=qemu-devel&m=133526669019403) after leaving lying
> > around for a while.
> > Next time do you want me to send xen_disk specific pull request to you
> > instead? Sorry if I shouldn't have sent the pull request directly...
> 
> I see. I wasn't aware that you're doing pull requests, but it isn't a
> problem. It just means that you should probably do a pull request for
> this one as well instead of expecting that I pick it up. But you can
> have my Acked-by, if you like.
> 
> Should I ignore xen_disk patches from now on for the block branch and
> assume that you pick them up?

I am happy to issue pull requests for xen_disk patches, but I still
welcome your reviews :-)

Patch hide | download patch | download mbox

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 4a6d89c..3e4a47b 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -48,7 +48,6 @@ 
 
 /* ------------------------------------------------------------- */
 
-static int syncwrite    = 0;
 static int batch_maps   = 0;
 
 static int max_requests = 32;
@@ -189,15 +188,10 @@  static int ioreq_parse(struct ioreq *ioreq)
             ioreq->presync = 1;
             return 0;
         }
-        if (!syncwrite) {
-            ioreq->postsync = 1;
-        }
+        ioreq->postsync = 1;
         /* fall through */
     case BLKIF_OP_WRITE:
         ioreq->prot = PROT_READ; /* from memory */
-        if (syncwrite) {
-            ioreq->postsync = 1;
-        }
         break;
     default:
         xen_be_printf(&blkdev->xendev, 0, "error: unknown operation (%d)\n",