Re: [PATCH V2 2/3] xen-blkfront: teach blkfront driver handle trim request

["Jan Beulich" <JBeulich@novell.com>]

>>> On 22.08.11 at 11:19, Li Dongyang <jerry87905@gmail.com> wrote:
> On Thu, Aug 18, 2011 at 6:18 PM, Jan Beulich <JBeulich@novell.com> wrote:
>>        >>> On 18.08.11 at 11:35, Li Dongyang <lidongyang@novell.com> wrote:
>>> @@ -1178,7 +1211,24 @@ static void blkfront_connect(struct blkfront_info
>>> *info)
>>>               info->feature_flush = REQ_FLUSH;
>>>               info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
>>>       }
>>> -
>>> +
>>> +     err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
>>> +                         "feature-trim", "%d", &trim,
>>> +                         NULL);
>>
>> So you switched this to use "trim", but ...
>>
>>> +
>>> +     if (!err && trim) {
>>> +             info->feature_trim = 1;
>>> +             err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
>>> +                     "discard_granularity", "%u", &discard_granularity,
>>> +                     "discard_alignment", "%u", &discard_alignment,
>>
>> ... you kept these using "discard" - quite inconsistent.
> the discard_granularity and discard_alignment are taken from struct 
> queue_limits
> they are needed to setup the queue in guest if we are using a phy
> device has trim.
> so I think we can keep the names here.

The way Linux names them doesn't matter for the xenstore interface.
I think they should be named so that the connection to the base
node's name is obvious.

>>
>> Also, I think (but I may be wrong) that dashes are preferred over
>> underscores in xenstore node names.
> that could be done in xenstore, I used dashes because they are taken
> from struct queue_limits
>>
>>> +                     NULL);
>>> +             if (!err) {
>>> +                     info->discard_granularity = discard_granularity;
>>> +                     info->discard_alignment = discard_alignment;
>>
>> I think you should set info->feature_trim only here (and then you can
>> eliminate the local variables).
> those discard_granularity and discard_alignment are only meaningful if
> the backend is a phy device has trim,
> and the 2 args are read from the queue limits from host. if the
> backend is a file, we can go with no discard_granularity
> and discard_alignment because we are about to punch a hole in the image 
> file.

Then you should update the condition accordingly. Setting
info->feature_trim prematurely is just calling for later problems.

Jan