Re: [Qemu-devel] [PATCH v5] sheepdog: add discard/trim support for sheepdog

[Liu Yuan <namei.unix@gmail.com>]

On 04/16/2013 04:47 PM, Kevin Wolf wrote:
> Am 16.04.2013 um 10:18 hat Stefan Hajnoczi geschrieben:
>> > On Tue, Apr 16, 2013 at 12:15:04AM +0800, Liu Yuan wrote:
>>> > > @@ -727,6 +730,20 @@ static void coroutine_fn aio_read_response(void *opaque)
>>> > >              rsp.result = SD_RES_SUCCESS;
>>> > >          }
>>> > >          break;
>>> > > +    case AIOCB_DISCARD_OBJ:
>>> > > +        switch (rsp.result) {
>>> > > +        case SD_RES_INVALID_PARMS:
>>> > > +            error_report("you are running the old sheep that doesn't support "
>>> > > +                         "discard command.\n");
>> > 
>> > error_report() does not need '\n'.
>> > 
>> > The recently added ssh block driver has a similar case when the server
>> > does not support fsync.  It does the following:
>> > 
>> > 1. Print the error message once only per volume, avoid filling up logs
>> >    on the host.

All the request for the volumes are firstly handled by the sheep daemon
this QEMU connects to, so we can say that if one discard request for any
volume return SD_RES_INVALID_PARMS, then all the volumes attatched to
this VM can't support discard operation.

>> > 2. Include details of the volume/server in case the users is connected
>> >    to multiple volumes/servers.  This allows them to figure out which
>> >    server is outdated.
>> > 

Multi-connections aren't supported yet (though planned), so this doesn't
apply for current SD.

>> > This makes the error messages safe from denial-of-service and includes
>> > more useful information.
> Or if we can check whether discard works during bdrv_open(), we could
> already fail there for discard=on.

Hmm, SD doesn't support a feature negotiation request. The most simple
way I can come up is add a s->enable_discard flag that set false when it
is reported discard operation isn't supported by the server connected.
What do you think?

Thanks,
Yuan