Xen devel - Jun 2008 - [PATCH 0/4]: Expand xvd to support > 16 devices

All,
     Current blktap and blkfront are limited to 16 xvd devices (xvda ->
xvdp).
This is enforced in the userland dom0 tools, but is also hard-coded into the
blkfront kernel code (even though modern dev_t can hold many more than 256
minors).  Based on the discussion that we had here:

http://lists.xensource.com/archives/html/xen-devel/2008-05/msg00128.html

I ended up implementing Ian Jackson''s suggestion here:

http://lists.xensource.com/archives/html/xen-devel/2008-05/msg00231.html

Basically, I left the old format alone, but added a new format that looks like:

 1 << 28 | disk << 8 | partition       xvd, disks or partitions 16
onwards

This format is used for any disks xvdq onward.  Note that blktap has a hardcoded
limit of 100 devices that I did not change with this patch series; if that ends
up being a problem, then that''s just a simple #define to change.

I did not expand the number of partitions available (it''s still 15),
although
there is space in the allocation to do that if someone wishes.  More details are
in each individual patch.

Note that I developed this against RHEL-5 kernels and ported it over to
xen-unstable, and only compile tested it there.

Chris Lalancette

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Chris Lalancette writes ("[Xen-devel] [PATCH 0/4]: Expand xvd to support
> 16 devices"):
> I ended up implementing Ian Jackson''s suggestion here:
>   http://lists.xensource.com/archives/html/xen-devel/2008-05/msg00231.html
> Basically, I left the old format alone, but added a new format that
> looks like:
> 
>  1 << 28 | disk << 8 | partition       xvd, disks or partitions
16 onwards
I approve of this, obviously.  But I think your patch lacks some error
checks.  These are most critical in the guest, as the guest''s
interpretation of the interface will effectively be frozen.

When the guest is enumerating the devices, it should be sure to
check that the block device number integer matches one of the expected
forms, as I wrote in my message.  If the number does not, then that
vbd should be ignored with a warning message.

This applies also to the partition numbers which you are currently
limiting to 15.  That''s fine but you should put in a check so that
out-of-range partition numbers are ignored rather than causing
unexpected behaviours.  (I''ll admit that I haven''t analysed
your code
in detail to determine exactly what the behaviour would be ...)

(Obviously even adding this check now won''t prevent attempts to
specify out of range devices from totally breaking even older guests
which lack proper checking.  But there''s no reason to perpetuate these
bugs.)

Also I think you should include an API changelog entry.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

(sorry for the delay in responding)

Ian Jackson wrote:
> When the guest is enumerating the devices, it should be sure to
> check that the block device number integer matches one of the expected
> forms, as I wrote in my message.  If the number does not, then that
> vbd should be ignored with a warning message.
OK, yes, I see, that makes sense.  I''ll make the appropriate change in
xlvbd_add().
> 
> This applies also to the partition numbers which you are currently
> limiting to 15.  That''s fine but you should put in a check so that
> out-of-range partition numbers are ignored rather than causing
> unexpected behaviours.  (I''ll admit that I haven''t
analysed your code
> in detail to determine exactly what the behaviour would be ...)
I''m not sure that this one is a problem (although I could be wrong). 
During
xlvbd_add() time, we end up doing an alloc_disk() with the number of minors that
we can use.  So I don''t think that the rest of the system will allow us
to go
beyond that value; empirical evidence seems to support this, as attaching a disk
with 16 partitions to /dev/xvdb only shows the first 15 partitions.

Incidentally, the comment I made in my initial posting about expanding the
partitions is wrong, looking back at the code.  I *did* expand the number of
entries that blkfront will pick up (i.e. increased nr_minors when doing the
alloc_disk()), but I did not change the tools side to accept partitions > 15.
Again, something that can easily be done in the future.
> Also I think you should include an API changelog entry.
Do you mean on the Xen Wiki?  I did find a page about API changes, so if/when
these patches go in, I''m happy to add an entry there.

Thanks for looking,
Chris Lalancette

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 25/6/08 10:45, "Chris Lalancette" <clalance@redhat.com>
wrote:
>> Also I think you should include an API changelog entry.
> 
> Do you mean on the Xen Wiki?  I did find a page about API changes, so
if/when
> these patches go in, I''m happy to add an entry there.
docs/ChangeLog in the repository.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel