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

[MORITA Kazutaka]

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);
 
	int bsize = get_objsize(oid) /  (sizeof(uint64_t) * BITS_PER_BYTE);

	assert(is_vdi_obj(oid) || sector_aligned(bsize));

	return bsize;
}

Thanks,

Kazutaka