Xen devel - Mar 2011 - [PATCH 2/12] VTPM mini-os: posix IO layer for blkfront in mini-os

The following patch adds POSIX IO for the blkfront device driver.

If mini-os is built with #define HAVE_LIBC then one can use 
blkfront_open() to obtain a file descriptor to a block device. read(), 
write(), lseek() and fstat() can be used on the file descriptor to read 
and write to the block device in a syncronous manner.

Signed off by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>



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

Matthew Fioravante, le Fri 11 Mar 2011 17:34:26 -0500, a écrit
:
> +      /*Make sure we have write permission */
> +      if(dev->info.info & VDISK_READONLY || dev->info.mode !=
O_RDWR) {
O_WRONLY too.
> +   aiocb.aio_dev = dev;
> +   aiocb.aio_buf = _xmalloc(blocksize, dev->info.sector_size);
> +   aiocb.aio_nbytes = blocksize;
> +   aiocb.aio_offset = blknum * blocksize;
> +   aiocb.aio_cb = NULL;
> +   aiocb.data = NULL;
> +
> +      /* read operation */
> +      if(!write) {
> +	 aiocb.aio_cb = NULL;
> +	 blkfront_read(&aiocb);
> +	 memcpy(buf, &aiocb.aio_buf[blkoff], bytes);
> +      } 
Could you perhaps optimize when buf is actually aligned?  That would
save a copy.
> +      /* Write operation */
> +      else {
> +	 /* If we''re writing a partial block, we need to read the
current contents first
> +	  * so we don''t overwrite the extra bits with garbage */
> +	 if(blkoff != 0 || bytes < blocksize) {
> +	    aiocb.aio_cb = NULL;
Maybe blkfront_aio_cb should do it itself?  It looks odd to have to do
it when reusing an aiocb structure.

Apart from that it looks good.

Samuel

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

On 03/11/2011 08:05 PM, Samuel Thibault wrote:
> Matthew Fioravante, le Fri 11 Mar 2011 17:34:26 -0500, a écrit :
>> +      /*Make sure we have write permission */
>> +      if(dev->info.info&  VDISK_READONLY || dev->info.mode
!= O_RDWR) {
> O_WRONLY too.
Good catch, actually testing a bitfield with != is a bad idea to begin 
with anyway.
>> +   aiocb.aio_dev = dev;
>> +   aiocb.aio_buf = _xmalloc(blocksize, dev->info.sector_size);
>> +   aiocb.aio_nbytes = blocksize;
>> +   aiocb.aio_offset = blknum * blocksize;
>> +   aiocb.aio_cb = NULL;
>> +   aiocb.data = NULL;
>> +
>> +      /* read operation */
>> +      if(!write) {
>> +	 aiocb.aio_cb = NULL;
>> +	 blkfront_read(&aiocb);
>> +	 memcpy(buf,&aiocb.aio_buf[blkoff], bytes);
>> +      }
> Could you perhaps optimize when buf is actually aligned?  That would
> save a copy.
>
This can be done but only if in the current iteration of the loop an 
entire block is being read. Since aiocb only operates on sectors it''ll 
read at minimum a whole sector into buf. If buf isnt big enough to hold 
the data a secondary buffer with a copy operation will have to be
done.
>> +      /* Write operation */
>> +      else {
>> +	 /* If we''re writing a partial block, we need to read the
current contents first
>> +	  * so we don''t overwrite the extra bits with garbage */
>> +	 if(blkoff != 0 || bytes<  blocksize) {
>> +	    aiocb.aio_cb = NULL;
> Maybe blkfront_aio_cb should do it itself?  It looks odd to have to do
> it when reusing an aiocb structure.
>
It could, but then that changes the design of aiocb. Was it supposed to 
be a very low level interface for just reading and writing blocks onto 
the disk?

Right now you have to set aio_nbytes and aio_offset to a multiple of 
sector size. This could be changed to allow variable sizes. 
Alternatively 2 new fields could be added to specify which portion 
inside a block to operate on.

Can you send a partial block through the xen block frontend and backend 
interface? If not we would have to queue up a read and then a write 
internally when the user requests a write. Its possible some users may 
not want this forced behavior of 2 operations.
> Apart from that it looks good.
>
> Samuel



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

Matthew Fioravante, le Mon 14 Mar 2011 15:07:30 -0400, a écrit
:
> On 03/11/2011 08:05 PM, Samuel Thibault wrote:
> >Matthew Fioravante, le Fri 11 Mar 2011 17:34:26 -0500, a écrit :
> >>+      /*Make sure we have write permission */
> >>+      if(dev->info.info&  VDISK_READONLY ||
dev->info.mode != O_RDWR) {
> >O_WRONLY too.
> Good catch, actually testing a bitfield with != is a bad idea to begin 
> with anyway.
Err, it''s not a bitfield, in mini-os it''s a {0,1,2} enum.
> >Could you perhaps optimize when buf is actually aligned?  That would
> >save a copy.
> >
> This can be done but only if in the current iteration of the loop an 
> entire block is being read.
Sure.
> Since aiocb only operates on sectors it''ll 
> read at minimum a whole sector into buf. If buf isnt big enough to hold 
> the data a secondary buffer with a copy operation will have to be done.
Sure.

But I expect people using that interface to tend to allocate big aligned
buffers.
> >>+      /* Write operation */
> >>+      else {
> >>+	 /* If we''re writing a partial block, we need to read
the current
> >>contents first
> >>+	  * so we don''t overwrite the extra bits with garbage */
> >>+	 if(blkoff != 0 || bytes<  blocksize) {
> >>+	    aiocb.aio_cb = NULL;
> >Maybe blkfront_aio_cb should do it itself?  It looks odd to have to do
> >it when reusing an aiocb structure.
> >
> It could, but then that changes the design of aiocb. Was it supposed to 
> be a very low level interface for just reading and writing blocks onto 
> the disk?
Well, I''d say the whole blkfront itself is a low-level interrface, and
your patch actually provides a higher one :)

This particular change in the design shouldn''t break anything, since
aio_cb is actually filled by blkfront itself, so it makes sense that it
cleans it since it expects it to be cleaned.
> Right now you have to set aio_nbytes and aio_offset to a multiple of 
> sector size. This could be changed to allow variable sizes. 
> Alternatively 2 new fields could be added to specify which portion 
> inside a block to operate on.
> 
> Can you send a partial block through the xen block frontend and backend 
> interface?
No.
> If not we would have to queue up a read and then a write 
> internally when the user requests a write. Its possible some users may 
> not want this forced behavior of 2 operations.
That''s why I wouldn''t recommend aio_nbytes/offset to be
allowed to
be non-multiples of the sector size. That interface is meant to be
an efficient sector-transfer interface. Your posix layer can handle
flexibility for the user.

Samuel

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

I think I totally misunderstood you. With the last case are you just 
referring to how I keep having to set the callback to NULL everytime? If 
so I totally agree with you. It should just set it back to NULL when its 
done.

I thought you were talking about how I was handling writes to partial 
blocks and that somehow the low level blkfront functions should be doing 
this transparently.

On 03/14/2011 03:29 PM, Samuel Thibault wrote:
> Matthew Fioravante, le Mon 14 Mar 2011 15:07:30 -0400, a écrit :
>> On 03/11/2011 08:05 PM, Samuel Thibault wrote:
>>> Matthew Fioravante, le Fri 11 Mar 2011 17:34:26 -0500, a écrit :
>>>> +      /*Make sure we have write permission */
>>>> +      if(dev->info.info&   VDISK_READONLY ||
dev->info.mode != O_RDWR) {
>>> O_WRONLY too.
>> Good catch, actually testing a bitfield with != is a bad idea to begin
>> with anyway.
> Err, it''s not a bitfield, in mini-os it''s a {0,1,2} enum.
>
>>> Could you perhaps optimize when buf is actually aligned?  That
would
>>> save a copy.
>>>
>> This can be done but only if in the current iteration of the loop an
>> entire block is being read.
> Sure.
>
>> Since aiocb only operates on sectors it''ll
>> read at minimum a whole sector into buf. If buf isnt big enough to hold
>> the data a secondary buffer with a copy operation will have to be done.
> Sure.
>
> But I expect people using that interface to tend to allocate big aligned
> buffers.
>
>>>> +      /* Write operation */
>>>> +      else {
>>>> +	 /* If we''re writing a partial block, we need to
read the current
>>>> contents first
>>>> +	  * so we don''t overwrite the extra bits with
garbage */
>>>> +	 if(blkoff != 0 || bytes<   blocksize) {
>>>> +	    aiocb.aio_cb = NULL;
>>> Maybe blkfront_aio_cb should do it itself?  It looks odd to have to
do
>>> it when reusing an aiocb structure.
>>>
>> It could, but then that changes the design of aiocb. Was it supposed to
>> be a very low level interface for just reading and writing blocks onto
>> the disk?
> Well, I''d say the whole blkfront itself is a low-level interrface,
and
> your patch actually provides a higher one :)
>
> This particular change in the design shouldn''t break anything,
since
> aio_cb is actually filled by blkfront itself, so it makes sense that it
> cleans it since it expects it to be cleaned.
>
>> Right now you have to set aio_nbytes and aio_offset to a multiple of
>> sector size. This could be changed to allow variable sizes.
>> Alternatively 2 new fields could be added to specify which portion
>> inside a block to operate on.
>>
>> Can you send a partial block through the xen block frontend and backend
>> interface?
> No.
>
>> If not we would have to queue up a read and then a write
>> internally when the user requests a write. Its possible some users may
>> not want this forced behavior of 2 operations.
> That''s why I wouldn''t recommend aio_nbytes/offset to be
allowed to
> be non-multiples of the sector size. That interface is meant to be
> an efficient sector-transfer interface. Your posix layer can handle
> flexibility for the user.
>
> Samuel



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

Matthew Fioravante, le Mon 14 Mar 2011 17:32:31 -0400, a écrit
:
> >>>>+	    aiocb.aio_cb = NULL;
> >>>Maybe blkfront_aio_cb should do it itself?  It looks odd to
have to do
> >>>it when reusing an aiocb structure.
> >>>
> I think I totally misunderstood you. With the last case are you just 
> referring to how I keep having to set the callback to NULL everytime?
Yes.
> If so I totally agree with you. It should just set it back to NULL
> when its done.
> 
> I thought you were talking about how I was handling writes to partial 
> blocks and that somehow the low level blkfront functions should be doing 
> this transparently.
Nope.

Samuel

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