* [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device
@ 2014-07-07 18:38 Anand Jain
2014-07-08 3:34 ` Miao Xie
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Anand Jain @ 2014-07-07 18:38 UTC (permalink / raw)
To: linux-btrfs
(for review comments pls).
btrfs_scan_one_device() needs SB, instead of doing it from scratch could
use btrfs_get_bdev_and_sb()
Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
fs/btrfs/volumes.c | 51 ++++++---------------------------------------------
1 file changed, 6 insertions(+), 45 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c166355..94e6131 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1053,14 +1053,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
{
struct btrfs_super_block *disk_super;
struct block_device *bdev;
- struct page *page;
- void *p;
int ret = -EINVAL;
u64 devid;
u64 transid;
u64 total_devices;
- u64 bytenr;
- pgoff_t index;
+ struct buffer_head *bh;
/*
* we would like to check all the supers, but that would make
@@ -1068,44 +1065,12 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
* So, we need to add a special mount option to scan for
* later supers, using BTRFS_SUPER_MIRROR_MAX instead
*/
- bytenr = btrfs_sb_offset(0);
mutex_lock(&uuid_mutex);
- bdev = blkdev_get_by_path(path, flags, holder);
-
- if (IS_ERR(bdev)) {
- ret = PTR_ERR(bdev);
+ ret = btrfs_get_bdev_and_sb(path, flags, holder, 0, &bdev, &bh);
+ if (ret)
goto error;
- }
-
- /* make sure our super fits in the device */
- if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
- goto error_bdev_put;
-
- /* make sure our super fits in the page */
- if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
- goto error_bdev_put;
-
- /* make sure our super doesn't straddle pages on disk */
- index = bytenr >> PAGE_CACHE_SHIFT;
- if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
- goto error_bdev_put;
-
- /* pull in the page with our super */
- page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
- index, GFP_NOFS);
-
- if (IS_ERR_OR_NULL(page))
- goto error_bdev_put;
-
- p = kmap(page);
-
- /* align our pointer to the offset of the super block */
- disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
-
- if (btrfs_super_bytenr(disk_super) != bytenr ||
- btrfs_super_magic(disk_super) != BTRFS_MAGIC)
- goto error_unmap;
+ disk_super = (struct btrfs_super_block *) bh->b_data;
devid = btrfs_stack_device_id(&disk_super->dev_item);
transid = btrfs_super_generation(disk_super);
@@ -1125,13 +1090,9 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
printk(KERN_CONT "devid %llu transid %llu %s\n", devid, transid, path);
}
-
-error_unmap:
- kunmap(page);
- page_cache_release(page);
-
-error_bdev_put:
+ brelse(bh);
blkdev_put(bdev, flags);
+
error:
mutex_unlock(&uuid_mutex);
return ret;
--
2.0.0.257.g75cc6c6
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device
2014-07-07 18:38 [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device Anand Jain
@ 2014-07-08 3:34 ` Miao Xie
2014-07-08 4:08 ` Liu Bo
2014-07-22 17:42 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: Miao Xie @ 2014-07-08 3:34 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On Tue, 8 Jul 2014 02:38:37 +0800, Anand Jain wrote:
> (for review comments pls).
>
> btrfs_scan_one_device() needs SB, instead of doing it from scratch could
> use btrfs_get_bdev_and_sb()
>
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> ---
> fs/btrfs/volumes.c | 51 ++++++---------------------------------------------
> 1 file changed, 6 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c166355..94e6131 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1053,14 +1053,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> {
> struct btrfs_super_block *disk_super;
> struct block_device *bdev;
> - struct page *page;
> - void *p;
> int ret = -EINVAL;
> u64 devid;
> u64 transid;
> u64 total_devices;
> - u64 bytenr;
> - pgoff_t index;
> + struct buffer_head *bh;
>
> /*
> * we would like to check all the supers, but that would make
> @@ -1068,44 +1065,12 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> * So, we need to add a special mount option to scan for
> * later supers, using BTRFS_SUPER_MIRROR_MAX instead
> */
> - bytenr = btrfs_sb_offset(0);
> mutex_lock(&uuid_mutex);
>
> - bdev = blkdev_get_by_path(path, flags, holder);
> -
> - if (IS_ERR(bdev)) {
> - ret = PTR_ERR(bdev);
> + ret = btrfs_get_bdev_and_sb(path, flags, holder, 0, &bdev, &bh);
> + if (ret)
> goto error;
> - }
> -
> - /* make sure our super fits in the device */
> - if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
> - goto error_bdev_put;
I think moving this check into btrfs_get_bdev_and_sb is better. The other is OK.
Thanks
Miao
> -
> - /* make sure our super fits in the page */
> - if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
> - goto error_bdev_put;
> -
> - /* make sure our super doesn't straddle pages on disk */
> - index = bytenr >> PAGE_CACHE_SHIFT;
> - if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
> - goto error_bdev_put;
> -
> - /* pull in the page with our super */
> - page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
> - index, GFP_NOFS);
> -
> - if (IS_ERR_OR_NULL(page))
> - goto error_bdev_put;
> -
> - p = kmap(page);
> -
> - /* align our pointer to the offset of the super block */
> - disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
> -
> - if (btrfs_super_bytenr(disk_super) != bytenr ||
> - btrfs_super_magic(disk_super) != BTRFS_MAGIC)
> - goto error_unmap;
> + disk_super = (struct btrfs_super_block *) bh->b_data;
>
> devid = btrfs_stack_device_id(&disk_super->dev_item);
> transid = btrfs_super_generation(disk_super);
> @@ -1125,13 +1090,9 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> printk(KERN_CONT "devid %llu transid %llu %s\n", devid, transid, path);
> }
>
> -
> -error_unmap:
> - kunmap(page);
> - page_cache_release(page);
> -
> -error_bdev_put:
> + brelse(bh);
> blkdev_put(bdev, flags);
> +
> error:
> mutex_unlock(&uuid_mutex);
> return ret;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device
2014-07-07 18:38 [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device Anand Jain
2014-07-08 3:34 ` Miao Xie
@ 2014-07-08 4:08 ` Liu Bo
2014-07-08 4:24 ` Miao Xie
2014-07-22 17:42 ` David Sterba
2 siblings, 1 reply; 6+ messages in thread
From: Liu Bo @ 2014-07-08 4:08 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
On Tue, Jul 08, 2014 at 02:38:37AM +0800, Anand Jain wrote:
> (for review comments pls).
>
> btrfs_scan_one_device() needs SB, instead of doing it from scratch could
> use btrfs_get_bdev_and_sb()
>
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> ---
> fs/btrfs/volumes.c | 51 ++++++---------------------------------------------
> 1 file changed, 6 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c166355..94e6131 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1053,14 +1053,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> {
> struct btrfs_super_block *disk_super;
> struct block_device *bdev;
> - struct page *page;
> - void *p;
> int ret = -EINVAL;
> u64 devid;
> u64 transid;
> u64 total_devices;
> - u64 bytenr;
> - pgoff_t index;
> + struct buffer_head *bh;
>
> /*
> * we would like to check all the supers, but that would make
> @@ -1068,44 +1065,12 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> * So, we need to add a special mount option to scan for
> * later supers, using BTRFS_SUPER_MIRROR_MAX instead
> */
> - bytenr = btrfs_sb_offset(0);
> mutex_lock(&uuid_mutex);
>
> - bdev = blkdev_get_by_path(path, flags, holder);
> -
> - if (IS_ERR(bdev)) {
> - ret = PTR_ERR(bdev);
> + ret = btrfs_get_bdev_and_sb(path, flags, holder, 0, &bdev, &bh);
> + if (ret)
> goto error;
> - }
> -
> - /* make sure our super fits in the device */
> - if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
> - goto error_bdev_put;
> -
> - /* make sure our super fits in the page */
> - if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
> - goto error_bdev_put;
> -
> - /* make sure our super doesn't straddle pages on disk */
> - index = bytenr >> PAGE_CACHE_SHIFT;
> - if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
> - goto error_bdev_put;
Apparently btrfs_get_bdev_and_sb() lacks the above two checks, otherwise looks good.
thanks,
-liubo
> -
> - /* pull in the page with our super */
> - page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
> - index, GFP_NOFS);
> -
> - if (IS_ERR_OR_NULL(page))
> - goto error_bdev_put;
> -
> - p = kmap(page);
> -
> - /* align our pointer to the offset of the super block */
> - disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
> -
> - if (btrfs_super_bytenr(disk_super) != bytenr ||
> - btrfs_super_magic(disk_super) != BTRFS_MAGIC)
> - goto error_unmap;
> + disk_super = (struct btrfs_super_block *) bh->b_data;
>
> devid = btrfs_stack_device_id(&disk_super->dev_item);
> transid = btrfs_super_generation(disk_super);
> @@ -1125,13 +1090,9 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> printk(KERN_CONT "devid %llu transid %llu %s\n", devid, transid, path);
> }
>
> -
> -error_unmap:
> - kunmap(page);
> - page_cache_release(page);
> -
> -error_bdev_put:
> + brelse(bh);
> blkdev_put(bdev, flags);
> +
> error:
> mutex_unlock(&uuid_mutex);
> return ret;
> --
> 2.0.0.257.g75cc6c6
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device
2014-07-08 4:08 ` Liu Bo
@ 2014-07-08 4:24 ` Miao Xie
0 siblings, 0 replies; 6+ messages in thread
From: Miao Xie @ 2014-07-08 4:24 UTC (permalink / raw)
To: bo.li.liu, Anand Jain; +Cc: linux-btrfs
On Tue, 8 Jul 2014 12:08:19 +0800, Liu Bo wrote:
> On Tue, Jul 08, 2014 at 02:38:37AM +0800, Anand Jain wrote:
>> (for review comments pls).
>>
>> btrfs_scan_one_device() needs SB, instead of doing it from scratch could
>> use btrfs_get_bdev_and_sb()
>>
>> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
>> ---
>> fs/btrfs/volumes.c | 51 ++++++---------------------------------------------
>> 1 file changed, 6 insertions(+), 45 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index c166355..94e6131 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1053,14 +1053,11 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>> {
>> struct btrfs_super_block *disk_super;
>> struct block_device *bdev;
>> - struct page *page;
>> - void *p;
>> int ret = -EINVAL;
>> u64 devid;
>> u64 transid;
>> u64 total_devices;
>> - u64 bytenr;
>> - pgoff_t index;
>> + struct buffer_head *bh;
>>
>> /*
>> * we would like to check all the supers, but that would make
>> @@ -1068,44 +1065,12 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>> * So, we need to add a special mount option to scan for
>> * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>> */
>> - bytenr = btrfs_sb_offset(0);
>> mutex_lock(&uuid_mutex);
>>
>> - bdev = blkdev_get_by_path(path, flags, holder);
>> -
>> - if (IS_ERR(bdev)) {
>> - ret = PTR_ERR(bdev);
>> + ret = btrfs_get_bdev_and_sb(path, flags, holder, 0, &bdev, &bh);
>> + if (ret)
>> goto error;
>> - }
>> -
>> - /* make sure our super fits in the device */
>> - if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
>> - goto error_bdev_put;
>> -
>> - /* make sure our super fits in the page */
>> - if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
>> - goto error_bdev_put;
>> -
>> - /* make sure our super doesn't straddle pages on disk */
>> - index = bytenr >> PAGE_CACHE_SHIFT;
>> - if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
>> - goto error_bdev_put;
>
> Apparently btrfs_get_bdev_and_sb() lacks the above two checks, otherwise looks good.
In fact, our disk_super size is constant and <= min page size (4K), and we are sure that
it is impossible that the super block is cross the block, so the above two checks are unnecessary.
Thanks
Miao
>
> thanks,
> -liubo
>
>> -
>> - /* pull in the page with our super */
>> - page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
>> - index, GFP_NOFS);
>> -
>> - if (IS_ERR_OR_NULL(page))
>> - goto error_bdev_put;
>> -
>> - p = kmap(page);
>> -
>> - /* align our pointer to the offset of the super block */
>> - disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
>> -
>> - if (btrfs_super_bytenr(disk_super) != bytenr ||
>> - btrfs_super_magic(disk_super) != BTRFS_MAGIC)
>> - goto error_unmap;
>> + disk_super = (struct btrfs_super_block *) bh->b_data;
>>
>> devid = btrfs_stack_device_id(&disk_super->dev_item);
>> transid = btrfs_super_generation(disk_super);
>> @@ -1125,13 +1090,9 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>> printk(KERN_CONT "devid %llu transid %llu %s\n", devid, transid, path);
>> }
>>
>> -
>> -error_unmap:
>> - kunmap(page);
>> - page_cache_release(page);
>> -
>> -error_bdev_put:
>> + brelse(bh);
>> blkdev_put(bdev, flags);
>> +
>> error:
>> mutex_unlock(&uuid_mutex);
>> return ret;
>> --
>> 2.0.0.257.g75cc6c6
>>
>> --
>> 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
> --
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device
2014-07-07 18:38 [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device Anand Jain
2014-07-08 3:34 ` Miao Xie
2014-07-08 4:08 ` Liu Bo
@ 2014-07-22 17:42 ` David Sterba
2014-07-23 9:11 ` Anand Jain
2 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2014-07-22 17:42 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
On Tue, Jul 08, 2014 at 02:38:37AM +0800, Anand Jain wrote:
> (for review comments pls).
>
> btrfs_scan_one_device() needs SB, instead of doing it from scratch could
> use btrfs_get_bdev_and_sb()
Please see
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6f60cbd3ae442cb35861bb522f388db123d42ec1
your patch actually reverts the changes. btrfs_get_bdev_and_sb does not
work and it was a pretty nasty bug, we've spent a long afternoon
catching it.
Some of the check may seem unnecessary, but they 'make sure' that
something unexpected will not happen.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-23 9:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-07 18:38 [PATCH RFC] btrfs: code optimize use btrfs_get_bdev_and_sb() at btrfs_scan_one_device Anand Jain
2014-07-08 3:34 ` Miao Xie
2014-07-08 4:08 ` Liu Bo
2014-07-08 4:24 ` Miao Xie
2014-07-22 17:42 ` David Sterba
2014-07-23 9:11 ` Anand Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).