[Ocfs2-devel] [PATCH 13/15] ocfs2: ocfs2_group_bitmap_size has to handle old volume.

[Tao Ma]

Hi Joel,

Joel Becker wrote:
> On Thu, Apr 01, 2010 at 12:22:56PM +0800, Tao Ma wrote:
>> Joel Becker wrote:
>>> On Thu, Apr 01, 2010 at 10:59:10AM +0800, Tao Ma wrote:
>>>> ocfs2_group_bitmap_size has to handle the case when the
>>>> volume don't have discontiguous block group support.
>>> 	NAK.  We specify 256 in all cases.  This doesn't hurt old code
>>> because we've never used more bits.  Even though our old bg_size values
>>> were much larger, we never used that space because bg_bits was set to
>>> the smaller value.
>> No, it hurts.
>> So with an old ocfs2-tools and a new kernel, the new kernel will set
>> it to 256 while the old ocfs2-tools refused to work by checking
>> bg_size(see
>> chainalloc_process_group in libocfs2).
> 
> *facepalm*
> 
> 	Well, we shot ourselves in the foot.  The entire point of having
> the size stored is so that we can vary it and the code still works.
> Consider that "validation" of bg_size a bug.  The correct validation is
> ((bg_bits + 7) / 8) <= bg_size.
> 	So we do need the kernel to store the old-style bg_size in order
> to keep compatibility.  But we can do better.  First, you don't need the
> supports_discontig_bg argument.  You already have the superblock, just
> check ocfs2_supports_discontig_bg(OCFS2_SB(sb)).
I have thought of this before, but finally I gave up. The reason is that 
OCFS2_SB(sb) use ocfs2_super which is declared in ocfs2.h. So you see, I 
don't want to overlap these 2 head files.
> 	Secondly, that function needs a big fat comment.
> 
> static inline int ocfs2_group_bitmap_size(struct super_block *sb,
> 					  int suballocator)
> {
> 	int size = sb->s_blocksize -
> 		offsetof(struct ocfs2_group_desc, bg_bitmap);
> 
> 	/*
> 	 * The cluster allocator uses the entire block.  Suballocators have
> 	 * never used more than OCFS2_MAX_BG_BITMAP_SIZE.  Unfortunately, older
> 	 * code expects bg_size set to the maximum.  Thus we must keep
> 	 * bg_size as-is unless discontig_bg is enabled.
> 	 */
> 	if (suballocator && ocfs2_supports_discontig_bg(OCFS2_SB(sb)))
> 		size = OCFS2_MAX_BG_BITMAP_SIZE;
yes, I can add the comments.

Regards,
Tao