| Message ID | 1430384845-9666-1-git-send-email-xuw2015@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
General comment: everywhere the superblock "4096" is used, the right replacement is BTRFS_SUPER_INFO_SIZE On Thu, Apr 30, 2015 at 05:07:23PM +0800, xuw2015@gmail.com wrote: > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2548,8 +2548,8 @@ int open_ctree(struct super_block *sb, > btrfs_init_balance(fs_info); > btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work); > > - sb->s_blocksize = 4096; > - sb->s_blocksize_bits = blksize_bits(4096); > + sb->s_blocksize = BTRFS_BLOCK_SIZE; > + sb->s_blocksize_bits = blksize_bits(BTRFS_BLOCK_SIZE); > sb->s_bdi = &fs_info->bdi; > > btrfs_init_btree_inode(fs_info, tree_root); > @@ -3144,7 +3144,7 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) > if (bytenr + BTRFS_SUPER_INFO_SIZE >= > i_size_read(bdev->bd_inode)) > break; > - bh = __bread(bdev, bytenr / 4096, > + bh = __bread(bdev, bytenr / BTRFS_BLOCK_SIZE, That one is really BTRFS_SUPER_INFO_SIZE > BTRFS_SUPER_INFO_SIZE); > if (!bh) > continue; > @@ -3199,7 +3199,7 @@ static int write_dev_supers(struct btrfs_device *device, > break; > > if (wait) { > - bh = __find_get_block(device->bdev, bytenr / 4096, > + bh = __find_get_block(device->bdev, bytenr / BTRFS_BLOCK_SIZE, same here > BTRFS_SUPER_INFO_SIZE); > if (!bh) { > errors++; > @@ -3229,7 +3229,7 @@ static int write_dev_supers(struct btrfs_device *device, > * one reference for us, and we leave it for the > * caller > */ > - bh = __getblk(device->bdev, bytenr / 4096, > + bh = __getblk(device->bdev, bytenr / BTRFS_BLOCK_SIZE, same here > BTRFS_SUPER_INFO_SIZE); > if (!bh) { > printk(KERN_ERR "BTRFS: couldn't get super " > @@ -3904,13 +3904,13 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > * The common minimum, we don't know if we can trust the nodesize/sectorsize > * items yet, they'll be verified later. Issue just a warning. > */ > - if (!IS_ALIGNED(btrfs_super_root(sb), 4096)) > + if (!IS_ALIGNED(btrfs_super_root(sb), BTRFS_BLOCK_SIZE)) > printk(KERN_WARNING "BTRFS: tree_root block unaligned: %llu\n", > btrfs_super_root(sb)); > - if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096)) > + if (!IS_ALIGNED(btrfs_super_chunk_root(sb), BTRFS_BLOCK_SIZE)) > printk(KERN_WARNING "BTRFS: chunk_root block unaligned: %llu\n", > btrfs_super_chunk_root(sb)); > - if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096)) > + if (!IS_ALIGNED(btrfs_super_log_root(sb), BTRFS_BLOCK_SIZE)) > printk(KERN_WARNING "BTRFS: log_root block unaligned: %llu\n", > btrfs_super_log_root(sb)); Throughout btrfs_check_super_valid 4096 is used as a minimum block size and the macro should be more descriptive. > > @@ -3918,14 +3918,14 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > * Check the lower bound, the alignment and other constraints are > * checked later. > */ > - if (btrfs_super_nodesize(sb) < 4096) { > - printk(KERN_ERR "BTRFS: nodesize too small: %u < 4096\n", > - btrfs_super_nodesize(sb)); > + if (btrfs_super_nodesize(sb) < BTRFS_BLOCK_SIZE) { > + printk(KERN_ERR "BTRFS: nodesize too small: %u < %d\n", > + btrfs_super_nodesize(sb), BTRFS_BLOCK_SIZE); > ret = -EINVAL; > } > - if (btrfs_super_sectorsize(sb) < 4096) { > - printk(KERN_ERR "BTRFS: sectorsize too small: %u < 4096\n", > - btrfs_super_sectorsize(sb)); > + if (btrfs_super_sectorsize(sb) < BTRFS_BLOCK_SIZE) { > + printk(KERN_ERR "BTRFS: sectorsize too small: %u < %d\n", > + btrfs_super_sectorsize(sb), BTRFS_BLOCK_SIZE); > ret = -EINVAL; > } > > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index d4cbfee..4d246ff 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -19,8 +19,9 @@ > #ifndef __DISKIO__ > #define __DISKIO__ > > +#define BTRFS_BLOCK_SIZE 4096 > #define BTRFS_SUPER_INFO_OFFSET (64 * 1024) > -#define BTRFS_SUPER_INFO_SIZE 4096 > +#define BTRFS_SUPER_INFO_SIZE BTRFS_BLOCK_SIZE This should stay 4096, super info size is defined as 4096, block size can vary according to page size. IOW, I don't see the point to introduce BTRFS_BLOCK_SIZE, it should be something like BTRFS_MIN_BLOCK_SIZE and used in the validation functions. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for review. On Tue, May 5, 2015 at 11:34 PM, David Sterba <dsterba@suse.cz> wrote: > General comment: everywhere the superblock "4096" is used, the right > replacement is BTRFS_SUPER_INFO_SIZE > I think it depends on the context. BTRFS_SUPER_INFO_SIZE means the super block size for btrfs, which may not be considered as block size logically. > On Thu, Apr 30, 2015 at 05:07:23PM +0800, xuw2015@gmail.com wrote: >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -2548,8 +2548,8 @@ int open_ctree(struct super_block *sb, >> btrfs_init_balance(fs_info); >> btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work); >> >> - sb->s_blocksize = 4096; >> - sb->s_blocksize_bits = blksize_bits(4096); >> + sb->s_blocksize = BTRFS_BLOCK_SIZE; >> + sb->s_blocksize_bits = blksize_bits(BTRFS_BLOCK_SIZE); >> sb->s_bdi = &fs_info->bdi; >> >> btrfs_init_btree_inode(fs_info, tree_root); >> @@ -3144,7 +3144,7 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) >> if (bytenr + BTRFS_SUPER_INFO_SIZE >= >> i_size_read(bdev->bd_inode)) >> break; >> - bh = __bread(bdev, bytenr / 4096, >> + bh = __bread(bdev, bytenr / BTRFS_BLOCK_SIZE, > > That one is really BTRFS_SUPER_INFO_SIZE Here we call the __bread->__bread_gfp->__getblk_gfp, and the block is based on bdev->bd_inode->i_blkbits, which is initialized by set_blocksize in volumes.c of btrfs codes. This means the param block is the block nr based on the bdev, not the BTRFS_SUPER_INFO_SIZE.(I mean logically, because not all of them are 4096) So I think "bytenr / BTRFS_MIN_BLOCK_SIZE" or "bytenr / block_size(bdev)" may be more accurate. > >> BTRFS_SUPER_INFO_SIZE); >> if (!bh) >> continue; >> @@ -3199,7 +3199,7 @@ static int write_dev_supers(struct btrfs_device *device, >> break; >> >> if (wait) { >> - bh = __find_get_block(device->bdev, bytenr / 4096, >> + bh = __find_get_block(device->bdev, bytenr / BTRFS_BLOCK_SIZE, > > same here The __find_get_block also in the same context, the 2nd param block nr is based on the blocksize of bdev. > >> BTRFS_SUPER_INFO_SIZE); >> if (!bh) { >> errors++; >> @@ -3229,7 +3229,7 @@ static int write_dev_supers(struct btrfs_device *device, >> * one reference for us, and we leave it for the >> * caller >> */ >> - bh = __getblk(device->bdev, bytenr / 4096, >> + bh = __getblk(device->bdev, bytenr / BTRFS_BLOCK_SIZE, > > same here I think here is the same. > >> BTRFS_SUPER_INFO_SIZE); >> if (!bh) { >> printk(KERN_ERR "BTRFS: couldn't get super " >> @@ -3904,13 +3904,13 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, >> * The common minimum, we don't know if we can trust the nodesize/sectorsize >> * items yet, they'll be verified later. Issue just a warning. >> */ >> - if (!IS_ALIGNED(btrfs_super_root(sb), 4096)) >> + if (!IS_ALIGNED(btrfs_super_root(sb), BTRFS_BLOCK_SIZE)) >> printk(KERN_WARNING "BTRFS: tree_root block unaligned: %llu\n", >> btrfs_super_root(sb)); >> - if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096)) >> + if (!IS_ALIGNED(btrfs_super_chunk_root(sb), BTRFS_BLOCK_SIZE)) >> printk(KERN_WARNING "BTRFS: chunk_root block unaligned: %llu\n", >> btrfs_super_chunk_root(sb)); >> - if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096)) >> + if (!IS_ALIGNED(btrfs_super_log_root(sb), BTRFS_BLOCK_SIZE)) >> printk(KERN_WARNING "BTRFS: log_root block unaligned: %llu\n", >> btrfs_super_log_root(sb)); > > Throughout btrfs_check_super_valid 4096 is used as a minimum block size > and the macro should be more descriptive. Yes, I will convert the BTRFS_BLOCK_SIZE to BTRFS_MIN_BLOCK_SIZE. >> >> @@ -3918,14 +3918,14 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, >> * Check the lower bound, the alignment and other constraints are >> * checked later. >> */ >> - if (btrfs_super_nodesize(sb) < 4096) { >> - printk(KERN_ERR "BTRFS: nodesize too small: %u < 4096\n", >> - btrfs_super_nodesize(sb)); >> + if (btrfs_super_nodesize(sb) < BTRFS_BLOCK_SIZE) { >> + printk(KERN_ERR "BTRFS: nodesize too small: %u < %d\n", >> + btrfs_super_nodesize(sb), BTRFS_BLOCK_SIZE); >> ret = -EINVAL; >> } >> - if (btrfs_super_sectorsize(sb) < 4096) { >> - printk(KERN_ERR "BTRFS: sectorsize too small: %u < 4096\n", >> - btrfs_super_sectorsize(sb)); >> + if (btrfs_super_sectorsize(sb) < BTRFS_BLOCK_SIZE) { >> + printk(KERN_ERR "BTRFS: sectorsize too small: %u < %d\n", >> + btrfs_super_sectorsize(sb), BTRFS_BLOCK_SIZE); >> ret = -EINVAL; >> } >> >> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h >> index d4cbfee..4d246ff 100644 >> --- a/fs/btrfs/disk-io.h >> +++ b/fs/btrfs/disk-io.h >> @@ -19,8 +19,9 @@ >> #ifndef __DISKIO__ >> #define __DISKIO__ >> >> +#define BTRFS_BLOCK_SIZE 4096 >> #define BTRFS_SUPER_INFO_OFFSET (64 * 1024) >> -#define BTRFS_SUPER_INFO_SIZE 4096 >> +#define BTRFS_SUPER_INFO_SIZE BTRFS_BLOCK_SIZE > > This should stay 4096, super info size is defined as 4096, block size > can vary according to page size. > Yes, the BTRFS_SUPER_INFO_SIZE should irrelevant to the BTRFS_BLOCK_SIZE. > IOW, I don't see the point to introduce BTRFS_BLOCK_SIZE, it should be > something like BTRFS_MIN_BLOCK_SIZE and used in the validation > functions. Yes, I will use the BTRFS_MIN_BLOCK_SIZE. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 2ef9a4b..342d4fc 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2548,8 +2548,8 @@ int open_ctree(struct super_block *sb, btrfs_init_balance(fs_info); btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work); - sb->s_blocksize = 4096; - sb->s_blocksize_bits = blksize_bits(4096); + sb->s_blocksize = BTRFS_BLOCK_SIZE; + sb->s_blocksize_bits = blksize_bits(BTRFS_BLOCK_SIZE); sb->s_bdi = &fs_info->bdi; btrfs_init_btree_inode(fs_info, tree_root); @@ -3144,7 +3144,7 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) if (bytenr + BTRFS_SUPER_INFO_SIZE >= i_size_read(bdev->bd_inode)) break; - bh = __bread(bdev, bytenr / 4096, + bh = __bread(bdev, bytenr / BTRFS_BLOCK_SIZE, BTRFS_SUPER_INFO_SIZE); if (!bh) continue; @@ -3199,7 +3199,7 @@ static int write_dev_supers(struct btrfs_device *device, break; if (wait) { - bh = __find_get_block(device->bdev, bytenr / 4096, + bh = __find_get_block(device->bdev, bytenr / BTRFS_BLOCK_SIZE, BTRFS_SUPER_INFO_SIZE); if (!bh) { errors++; @@ -3229,7 +3229,7 @@ static int write_dev_supers(struct btrfs_device *device, * one reference for us, and we leave it for the * caller */ - bh = __getblk(device->bdev, bytenr / 4096, + bh = __getblk(device->bdev, bytenr / BTRFS_BLOCK_SIZE, BTRFS_SUPER_INFO_SIZE); if (!bh) { printk(KERN_ERR "BTRFS: couldn't get super " @@ -3904,13 +3904,13 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, * The common minimum, we don't know if we can trust the nodesize/sectorsize * items yet, they'll be verified later. Issue just a warning. */ - if (!IS_ALIGNED(btrfs_super_root(sb), 4096)) + if (!IS_ALIGNED(btrfs_super_root(sb), BTRFS_BLOCK_SIZE)) printk(KERN_WARNING "BTRFS: tree_root block unaligned: %llu\n", btrfs_super_root(sb)); - if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096)) + if (!IS_ALIGNED(btrfs_super_chunk_root(sb), BTRFS_BLOCK_SIZE)) printk(KERN_WARNING "BTRFS: chunk_root block unaligned: %llu\n", btrfs_super_chunk_root(sb)); - if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096)) + if (!IS_ALIGNED(btrfs_super_log_root(sb), BTRFS_BLOCK_SIZE)) printk(KERN_WARNING "BTRFS: log_root block unaligned: %llu\n", btrfs_super_log_root(sb)); @@ -3918,14 +3918,14 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, * Check the lower bound, the alignment and other constraints are * checked later. */ - if (btrfs_super_nodesize(sb) < 4096) { - printk(KERN_ERR "BTRFS: nodesize too small: %u < 4096\n", - btrfs_super_nodesize(sb)); + if (btrfs_super_nodesize(sb) < BTRFS_BLOCK_SIZE) { + printk(KERN_ERR "BTRFS: nodesize too small: %u < %d\n", + btrfs_super_nodesize(sb), BTRFS_BLOCK_SIZE); ret = -EINVAL; } - if (btrfs_super_sectorsize(sb) < 4096) { - printk(KERN_ERR "BTRFS: sectorsize too small: %u < 4096\n", - btrfs_super_sectorsize(sb)); + if (btrfs_super_sectorsize(sb) < BTRFS_BLOCK_SIZE) { + printk(KERN_ERR "BTRFS: sectorsize too small: %u < %d\n", + btrfs_super_sectorsize(sb), BTRFS_BLOCK_SIZE); ret = -EINVAL; } diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index d4cbfee..4d246ff 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -19,8 +19,9 @@ #ifndef __DISKIO__ #define __DISKIO__ +#define BTRFS_BLOCK_SIZE 4096 #define BTRFS_SUPER_INFO_OFFSET (64 * 1024) -#define BTRFS_SUPER_INFO_SIZE 4096 +#define BTRFS_SUPER_INFO_SIZE BTRFS_BLOCK_SIZE #define BTRFS_SUPER_MIRROR_MAX 3 #define BTRFS_SUPER_MIRROR_SHIFT 12