[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/4] pvSCSI driver



Hi Steven-san,

On Tue, 24 Jun 2008 14:13:13 +0100
Steven Smith <steven.smith@xxxxxxxxxx> wrote:
> rather than anything architectural.  The only bit which worries me is
> the xenbus-level protocol for adding and removing LUNs, and I suspect
> that's just because I've not understood what you're actually doing.
> If you could briefly explain how you expect this to work, at the level
> of individual xenstore operations, I expect that would make things a
> lot clearer.

I will briefly explain the xenbus-level protocol below. 

I have to say first is that the protocol uses following two kinds of 
states, "XenBusState" and "DeviceState". 
(Please do not being confused, because the two states use similar 
notation (XenBusState*ing or XenBusState*ed) as state name.)

1.) XenbusState
    The XenBusState is used to control frontend's and backend's 
    automata.
    The newly defined two states, "XenBusStateReconfiguring" and 
    "XenBusStateReconfigured", are used for attach/detach protocol.

2.) DeviceState
    In addition to the XenBusState, we need another kind of state 
    for each LUN to be attached and detached. The "DeviceState" is 
    used for such a purpose.
    It uses "XenBusStateInitializing", "XenBusStateIntialized" and
    "XenBusStateConnected" in normal case, and they are stored at
    "vscsi-devs/dev-*/state" in xenstore.


Following is a protocol for attaching a LUN. Please note that error 
case is not described for simplicity.

xend                    backend                 frontend
-----------------------------------------------------------------------
set DeviceState to
  XenBusStateInitializing.
set XenBusState to
  XenBusStateReconfiguring.

                                                detect backend has
                                                  changed.
                                                set XenBusState to
                                                  XenBusStateReconfiguring.

                        detect frontend has
                          changed.
                        export a specified LUN.
                          (call scsi_device_lookup(), 
                          maintain translation table
                          and so on.)
                        set DeviceState to
                          XenBusStateInitialized.
                        set XenBusState to
                          XenBusStateReconfigured.

                                                detect backend has
                                                  changed.
                                                import the exported LUN.
                                                  (call scsi_add_device().)
                                                set DeviceState to
                                                  XenBusStateConnected.
                                                set XenBusState to
                                                  XenBusStateConnected.

                        detect frontend has
                          changed.
                        set DeviceState to
                          XenBusStateConnected.
                        set XenBusState to
                          XenBusStateConnected.
-----------------------------------------------------------------------



> > /* quoted scsi_lib.c/scsi_req_map_sg . */
> > static int requset_map_sg(struct request *rq, pending_req_t *pending_req, 
> > unsigned int count)
> > {
> I don't quite understand why it was necessary to duplicate all of this
> code.  Would you mind explaining why you were unable to use the
> existing implementation, please?

In general, each segment of scatter/gather may start at offset 
non-zero like following figure.

 segment #0     segemnt #1     segemnt #2
  |~~~~~~|       |~~~~~~|       |~~~~~~|
  |      |       |      |       |~~~~~~|
  |~~~~~~|       |______|       |      |
  | Data |       | Data |       | Data |
  |______|       |~~~~~~|       |      |
  |      |       |      |       |______|
  |______|       |______|       |______|

However, I suppose that the existing requset_map_sg() implementation
assumes following structure, segment #0 may start offset non-zero but
segment #1 and later should start at offset zero.

 segment #0     segemnt #1     segemnt #2
  |~~~~~~|       |~~~~~~|       |~~~~~~|
  |      |       |      |       |      |
  |~~~~~~|       |      |       |      |
  |      |       | Data |       | Data |
  | Data |       |      |       |      |
  |      |       |      |       |______|
  |______|       |______|       |______|

Of cource, we can create a later type of scatter/gather structure on
backend driver by copying data, however performance will be degraded
maybe.
On the other hand, we can also take another way that we define 
scatter/gather interface between frontend and backend (defined in
vscsiif.h) should be later type. However, it will spoil flexibility 
of the interface.

That is the reason why we use duplicate (and bit modify) the existing
implementation.

Could you please suggest another good way?


> Also, there's a typo in the name of the function.
> 
> >     struct request_queue *q = rq->q;
> >     int nr_pages;
> >     unsigned int nsegs = count;
> > 
> >     unsigned int data_len = 0, len, bytes, off;
> >     struct page *page;
> >     struct bio *bio = NULL;
> >     int i, err, nr_vecs = 0;
> > 
> >     for (i = 0; i < nsegs; i++) {
> >             page = pending_req->sgl[i].page;
> >             off = (unsigned int)pending_req->sgl[i].offset;
> >             len = (unsigned int)pending_req->sgl[i].length;
> >             data_len += len;
> > 
> >             nr_pages = (len + off + PAGE_SIZE - 1) >> PAGE_SHIFT;
> >             while (len > 0) {
> >                     bytes = min_t(unsigned int, len, PAGE_SIZE - off);
> > 
> >                     if (!bio) {
> >                             nr_vecs = min_t(int, BIO_MAX_PAGES, nr_pages);
> >                             nr_pages -= nr_vecs;
> >                             bio = bio_alloc(GFP_KERNEL, nr_vecs);
> >                             if (!bio) {
> >                                     err = -ENOMEM;
> >                                     goto free_bios;
> >                             }
> >                             bio->bi_end_io = scsiback_bi_endio;
> >                     }
> > 
> >                     if (bio_add_pc_page(q, bio, page, bytes, off) !=
> >                                             bytes) {
> >                             bio_put(bio);
> >                             err = -EINVAL;
> >                             goto free_bios;
> >                     }
> > 
> >                     if (bio->bi_vcnt >= nr_vecs) {
> >                             err = scsiback_merge_bio(rq, bio);
> >                             if (err) {
> >                                     bio_endio(bio, bio->bi_size, 0);
> >                                     goto free_bios;
> >                             }
> >                             bio = NULL;
> >                     }
> > 
> >                     page++;
> >                     len -= bytes;
> >                     off = 0;
> >             }
> >     }
> > 
> >     rq->buffer   = rq->data = NULL;
> >     rq->data_len = data_len;
> > 
> >     return 0;
> > 
> > free_bios:
> >     while ((bio = rq->bio) != NULL) {
> >             rq->bio = bio->bi_next;
> >             /*
> >              * call endio instead of bio_put incase it was bounced
> >              */
> >             bio_endio(bio, bio->bi_size, 0);
> >     }
> > 
> >     return err;
> > }
> > 
> > #endif


Best regards,

-----
Jun Kamada



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.