[Sheepdog] [PATCH v3 2/2] sheep: abstract out store IO interface

[MORITA Kazutaka]

At Thu, 17 Nov 2011 18:56:38 +0800,
Liu Yuan wrote:
> 
> On 11/17/2011 06:52 PM, Liu Yuan wrote:
> 
> > On 11/17/2011 06:27 PM, MORITA Kazutaka wrote:
> > 
> >> I think this is still not generic...  If this is specific one to
> >> simple_store, move it to simple_store.c.
> >>
> >>
> > 
> > 
> > I use siocb for interface functions internal information passing *and*
> > for higher level code to specify flags, length, buf, offset to store.rw
> > too.
> > 
> > This is just a dirty hack, enabling me to make small changes to the
> > previous code to work with simple store correctly.
> > 
> > My intention is that siocb is not specific to simple store. we need a
> > channel to send information to store.open(), then it can use it
> > communicate with store.read/write/close.
> > 
> > So I think siocb would be globally seen by higher level code.
> > 
> > if we don't use siocb, then what do you suggest to use for higher level
> > information (for e.g, flags, length, buf, offset) to store's interfaces?
> > 
> > We can't simple pass a void *ptr, without knowing the internal structure
> > for it.

If you pass &ptr in store.open() and get a store driver specific
opaque, you don't need to know the internal structure.

> > 
> > and even if later, siocb get changed, we don't need to change current
> > (oid, void *priv) interface.
> > 
> > This is my own thought, maybe there is better idea.
> > 
> > 
> >> Remove the 'static' modifier in store.c if you want to use obj_path in
> >> this file.
> >>
> > 
> > 
> > Umm, obj_path is not static in my repo...I guess some patch remove it
> > already. I'll rebase it.
> > 
> > 
> >>> +			ret = store_write_last_sector(oid, &fd);
> >>
> >> store_write_last_sector() expects struct siocb in the second argument.
> >>
> > 
> > 
> > Good catch, it is my carelessness.
> > 
> >>
> >> I think we should define opaque as 'void *', get a store_driver
> >> specific opaque in the argument of store.open(), and pass it to
> >> store.read()/write()/close().  This is also what Christoph suggested.
> >> Am i wrong?
> >>
> > 
> > 
> > Like above, so it is totally opaque, where can we pass flags, length,
> > offset to store driver?
> > 
> 
> 
> And more, length, offset is critical for later partial data object
> read/write, if we support huge data size. So we can't assume an
> interface just need (key, value).

What I thought was:

struct store_driver {
       const char *driver_name;
       int (*init)(char *path);
       int (*open)(uint32_t epoch, uint64_t oid, int create, void **priv);
       int (*write)(uint64_t oid, void *buf, int len, off_t offset, void *priv);
       int (*read)(uint64_t oid, void *buf, int len, off_t offset, void *priv);
       int (*close)(void *priv);
};

On error, open/write/read returns an error code.  open() also returns
the driver specific data in the last argument.  read()/write() checks
whether data is fully read/written inside the method.

If you want to use siocb (I'm not sure this is the right way):

struct siocb {
       uint32_t epoch;
       void *buf;
       uint32_t length;
       uint64_t offset;
       int create
};

struct store_driver {
       const char *driver_name;
       int (*init)(char *path);
       int (*open)(uint64_t oid, struct siocb *cb, void **priv);
       int (*write)(uint64_t oid, struct siocb *cb, void *priv);
       int (*read)(uint64_t oid, struct siocb *cb, void *priv);
       int (*close)(void *priv);
};

'fd' shouldn't be included in siocb because it is simple_store specific.

Thanks,

Kazutaka