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

[Matthew Fioravante <matthew.fioravante@xxxxxxxxxx>]

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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

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