[sheepdog] [PATCH] cache: correct block size calculation for inode object

[Liu Yuan]

On Thu, Aug 08, 2013 at 12:33:23PM +0900, MORITA Kazutaka wrote:
> At Thu, 8 Aug 2013 10:02:59 +0800,
> Liu Yuan wrote:
> > 
> > On Thu, Aug 08, 2013 at 06:51:22AM +0900, MORITA Kazutaka wrote:
> > > At Wed,  7 Aug 2013 16:10:01 +0800,
> > > Liu Yuan wrote:
> > > > 
> > > > inode and data object have different sizes and thus cache block size is
> > > > different. Current code fail to get the right size for inode object when
> > > > its last bit of bmap is set with the fixed block size.
> > > > 
> > > > This patch tries to get the dynamic block size based on the oid.
> > > > 
> > > > Signed-off-by: Liu Yuan <namei.unix at gmail.com>
> > > > ---
> > > >  sheep/object_cache.c |   36 ++++++++++++++++--------------------
> > > >  1 file changed, 16 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/sheep/object_cache.c b/sheep/object_cache.c
> > > > index dcf6972..957b052 100644
> > > > --- a/sheep/object_cache.c
> > > > +++ b/sheep/object_cache.c
> > > > @@ -28,8 +28,6 @@
> > > >  
> > > >  #define CACHE_INDEX_MASK      (CACHE_CREATE_BIT)
> > > >  
> > > > -#define CACHE_BLOCK_SIZE      ((UINT64_C(1) << 10) * 64) /* 64 KB */
> > > > -
> > > >  #define CACHE_OBJECT_SIZE (SD_DATA_OBJ_SIZE / 1024 / 1024) /* M */
> > > >  
> > > >  /* Kick background pusher if dirty_count greater than it */
> > > > @@ -116,13 +114,19 @@ static inline bool idx_has_vdi_bit(uint32_t idx)
> > > >  	return !!(idx & CACHE_VDI_BIT);
> > > >  }
> > > >  
> > > > -static uint64_t calc_object_bmap(size_t len, off_t offset)
> > > > +static inline int get_cache_block_size(uint64_t oid)
> > > > +{
> > > > +	return DIV_ROUND_UP(get_objsize(oid), sizeof(uint64_t) * BITS_PER_BYTE);
> > > > +}
> > > 
> > > The size of cache block must be sector-aligned for O_DIRECT.  I'd
> > > suggest rounding up to 4 KB boundary (the size of blocks for most
> > > filesystems) like as follows.
> > > 
> > >   static inline int get_cache_block_size(uint64_t oid)
> > >   {
> > >       int bsize = DIV_ROUND_UP(get_objsize(oid), sizeof(uint64_t) * BITS_PER_BYTE);
> > > 
> > >       return round_up(bsize, BLOCK_SIZE);
> > >   }
> > 
> > data object is aiglied, and inode object isn't. But this is problem since we
> > always drop O_DIRECT for inode object, no?
> > 
> > If we round up size for inode object, we need much more code to handle last
> > block of inode object.
> 
> Hmm, yes, but your code is already calling DIV_ROUND_UP().  Currently,
> it's not a problem because (get_obj_size() % 64) is always zero, but
> the size of object may change in future.  Can you add some assert() to
> ensure the code correctness?  I guess a better code looks like as
> follows.
> 
> static inline int get_cache_block_size(uint64_t oid)
> {
> 	assert(get_objsize(oid) % (sizeof(uint64_t) * BITS_PER_BYTE) == 0);

This won't work. SD_INODE_SIZE can't divide 64. I think we can assume data
object is always aligned and handle partial last block of inode carefully.

Thanks
Yuan