qdisk vs. file as vbd type

qdisk vs. file as vbd type

[Olaf Hering]

What is the reason the backend 'type' property of a configured disk is
now "qdisk" instead of "file"? 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.
I wonder if that guest-visible change was well thought.

Olaf

Re: qdisk vs. file as vbd type

[Ian Campbell]

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.

> I wonder if that guest-visible change was well thought.
> 
> Olaf
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: qdisk vs. file as vbd type

[Olaf Hering]

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?

Olaf

Re: qdisk vs. file as vbd type

[Konrad Rzeszutek Wilk]

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.

> 
> Olaf

Re: qdisk vs. file as vbd type

[Olaf Hering]

[ removing Li who is no longer with Novell ]

On Fri, Jan 10, Konrad Rzeszutek Wilk wrote:

> 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.

If the backend cant do discard then it should not advertise the feature.

Are you ok with me sending a patch which simplifies this function?

Olaf

Re: qdisk vs. file as vbd type

[Konrad Rzeszutek Wilk]

On Fri, Jan 10, 2014 at 04:19:56PM +0100, Olaf Hering wrote:
> [ removing Li who is no longer with Novell ]
> 
> On Fri, Jan 10, Konrad Rzeszutek Wilk wrote:
> 
> > 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.
> 
> If the backend cant do discard then it should not advertise the feature.

<nods>
> 
> Are you ok with me sending a patch which simplifies this function?

Yes of course!
> 
> Olaf

Re: qdisk vs. file as vbd type

[Ian Campbell]

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.

Ian.

Re: qdisk vs. file as vbd type

[Konrad Rzeszutek Wilk]

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.
> 

Re: qdisk vs. file as vbd type

[Roger Pau Monné]

On 10/01/14 15:47, 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.


Linux blkfront checks for "feature-discard" on xenstore before trying 
to enable discard:

1795         err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
1796                             "feature-discard", "%d", &discard,
1797                             NULL);
1798 
1799         if (!err && discard)
1800                 blkfront_setup_discard(info);

But then in blkfront_setup_discard:

1615         type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
1616         if (IS_ERR(type))
1617                 return;
1618 
1619         info->feature_secdiscard = 0;
1620         if (strncmp(type, "phy", 3) == 0) {
1621                 err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
1622                         "discard-granularity", "%u", &discard_granularity,
1623                         "discard-alignment", "%u", &discard_alignment,
1624                         NULL);
1625                 if (!err) {
1626                         info->feature_discard = 1;
1627                         info->discard_granularity = discard_granularity;
1628                         info->discard_alignment = discard_alignment;
1629                 }
1630                 err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
1631                             "discard-secure", "%d", &discard_secure,
1632                             NULL);
1633                 if (!err)
1634                         info->feature_secdiscard = discard_secure;
1635 
1636         } else if (strncmp(type, "file", 4) == 0)
1637                 info->feature_discard = 1;

I don't understand why blkfront doesn't check directly for "discard-
granularity" and "discard-alignment" and setup discard based on those 
parameters. Also, even if the backend sets "feature-discard" but the 
type is not "phy" or "file" discard will not be set.

The commit message that introduced those changes contains no info 
regarding why discard is only enabled for "phy" or "file" backends. 
Ccing Konrad and the original author of the change.

Roger.