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

[MORITA Kazutaka]

At Thu, 8 Aug 2013 11:43:24 +0800,
Liu Yuan wrote:
> 
> 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.

Well, but your patch is not handling the partial at the last cache
block, no?  And if we have to handle the last cache block either way,
rounding up to the next 4KB boundary (more friendly size for
filesystem) sounds like no problem.

Thanks,

Kazutaka