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

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel