Re: qdisk vs. file as vbd type

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 10, 2014 at 03:20:45PM +0000, Ian Campbell wrote:
> On Fri, 2014-01-10 at 10:15 -0500, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 10, 2014 at 04:00:00PM +0100, Olaf Hering wrote:
> > > On Fri, Jan 10, Ian Campbell wrote:
> > > 
> > > > On Fri, 2014-01-10 at 15:40 +0100, Olaf Hering wrote:
> > > > > What is the reason the backend 'type' property of a configured disk is
> > > > > now "qdisk" instead of "file"?
> > > > 
> > > > Because qdisk is the backend instead of loop+blk (==file) I think this
> > > > just happens naturally.
> > > > 
> > > > >  Would the guest really care about that
> > > > > detail?  For example block-front currently just checks for "phy" and
> > > > > "file" when deciding if discard should be enabled.
> > > > 
> > > > That sounds entirely bogus, it should be checking for some sort of
> > > > feature-discard.
> > > 
> > > It does that, then calls blkfront_setup_discard which in turn knows just
> > > about phy and file. And I wonder why it does that.
> > > Maybe this function should be simplified to assume that if its called
> > > feature_discard can be enabled. And if both
> > > discard-granularity/discard-alignment exist those properties should be
> > > assigned, similar for discard-secure.
> > > 
> > > Now that I look at the history of blkfront_setup_discard:
> > > 
> > >  Li, Konrad, why does that function care at all about the 'type'?
> > >  Shouldnt that check be removed?
> > 
> > You are looking at:
> > ed30bf317       (Li Dongyang    2011-09-01 18:39:09 +0800       1664)   } else if (strncmp(type, "file", 4) == 0)
> > ed30bf317       (Li Dongyang    2011-09-01 18:39:09 +0800       1665)           info->feature_discard = 1;
> > ed30bf317       (Li Dongyang    2011-09-01 18:39:09 +0800       1666)
> > ed30bf317       (Li Dongyang    2011-09-01 18:39:09 +0800       1667)   kfree(type);
> > ed30bf317       (Li Dongyang    2011-09-01 18:39:09 +0800       1668)}
> > ed30bf317       (Li Dongyang    2011-09-01 18:39:09 +0800       1669)
> > 
> > My recollection is that at the time the patches were developed, loop
> > was not able to do discard operations. That has since changed and
> > loop can do it. Hence the force of =1 was added in.
> > 
> > But that assumes that 'file' is going through the 'loop' device.
> > 
> > If that assumption is incorrect then this needs to be fixed and
> > perhaps the underlaying device ('file'?) interogated as to whether
> > it can do discard or not.
> 
> Why on earth is this happening in the frontend?
> 
> The *backend* should be querying the underlying device and propagating
> the result via the feature flag to the frontend. Having the backend
> advertise discard and then have the frontend second guess this based on
> rumour and hearsay (which is all probing this type field) is just nuts.

Madness I say!

Note that the discard operations are OK with errors. That is if the disk
says 'I can do it' but returns -Exx whatever, the filesystems, tools
etc are OK with that.

But it is incorrect. Worst yet, why do we even check for 'type' to figure
this out is odd too.

> 
> Ian.
>