Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On Tue, Feb 13, 2018 at 11:00:46AM +0800, Anand Jain wrote: > Fixes the endianness bug in the fs_info::super_copy by using its > btrfs_set_super...() function to set values in the SB, as these > functions manage the endianness compatibility nicely. Reviewed-by: Liu Bo Thanks, -liubo > > Signed-off-by: Anand Jain > --- > v1->v2: Update change log. Update $Subject. > Old: > [PATCH] btrfs: use set functions to update latest refs to the SB > fs/btrfs/transaction.c | 20 > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 04f07144b45c..9220f004001c 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1722,19 +1722,23 @@ static void update_super_roots(struct btrfs_fs_info > *fs_info) > > super = fs_info->super_copy; > > + /* update latest btrfs_super_block::chunk_root refs */ > root_item = &fs_info->chunk_root->root_item; > - super->chunk_root = root_item->bytenr; > - super->chunk_root_generation = root_item->generation; > - super->chunk_root_level = root_item->level; > + btrfs_set_super_chunk_root(super, root_item->bytenr); > + btrfs_set_super_chunk_root_generation(super, root_item->generation); > + btrfs_set_super_chunk_root_level(super, root_item->level); > > + /* update latest btrfs_super_block::root refs */ > root_item = &fs_info->tree_root->root_item; > - super->root = root_item->bytenr; > - super->generation = root_item->generation; > - super->root_level = root_item->level; > + btrfs_set_super_root(super, root_item->bytenr); > + btrfs_set_super_generation(super, root_item->generation); > + btrfs_set_super_root_level(super, root_item->level); > + > if (btrfs_test_opt(fs_info, SPACE_CACHE)) > - super->cache_generation = root_item->generation; > + btrfs_set_super_cache_generation(super, root_item->generation); > if (test_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags)) > - super->uuid_tree_generation = root_item->generation; > + btrfs_set_super_uuid_tree_generation(super, > + root_item->generation); > } > > int btrfs_transaction_in_commit(struct btrfs_fs_info *info) > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On Wed, Feb 14, 2018 at 10:53:19PM +0800, Anand Jain wrote: > > > On 02/14/2018 01:55 AM, David Sterba wrote: > > On Tue, Feb 13, 2018 at 06:27:13PM +0800, Anand Jain wrote: > >> On 02/13/2018 05:01 PM, Qu Wenruo wrote: > >>> On 2018年02月13日 11:00, Anand Jain wrote: > Fixes the endianness bug in the fs_info::super_copy by using its > btrfs_set_super...() function to set values in the SB, as these > functions manage the endianness compatibility nicely. > > Signed-off-by: Anand Jain > >>> > >>> Also went through all btrfs_super_block SETGET functions, greping using > >>> \>, seems that there are still some left here: > >>> > >>> fs/btrfs/sysfs.c: > >>> In both btrfs_sectorsize_show() and btrfs_clone_alignment_show(): > >>> return snprintf(buf, PAGE_SIZE, "%u\n", > >>> fs_info->super_copy->sectorsize); > >>> > >>> In btrfs_nodesize_show(): > >>> return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->nodesize); > > Thinking again on the printf they are fine without the endianness > functions, because it is printing the values from the memory to > buf/stdout, which was previously read from the disk (which > possibly written by the opposite endianness system). Here, there > is no endianness changes that will be required for it to be written > out using printf. We need to fix the value for printf for the same reason this patch was needed: https://git.kernel.org/linus/bea7eafdbda3ba1d4b2ccb9cca829eefb7989bb9 and we can actually use the fs_info-> copies here too. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On 02/14/2018 01:55 AM, David Sterba wrote: On Tue, Feb 13, 2018 at 06:27:13PM +0800, Anand Jain wrote: On 02/13/2018 05:01 PM, Qu Wenruo wrote: On 2018年02月13日 11:00, Anand Jain wrote: Fixes the endianness bug in the fs_info::super_copy by using its btrfs_set_super...() function to set values in the SB, as these functions manage the endianness compatibility nicely. Signed-off-by: Anand Jain Also went through all btrfs_super_block SETGET functions, greping using \>, seems that there are still some left here: fs/btrfs/sysfs.c: In both btrfs_sectorsize_show() and btrfs_clone_alignment_show(): return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->sectorsize); In btrfs_nodesize_show(): return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->nodesize); Thinking again on the printf they are fine without the endianness functions, because it is printing the values from the memory to buf/stdout, which was previously read from the disk (which possibly written by the opposite endianness system). Here, there is no endianness changes that will be required for it to be written out using printf. Oh. Thanks. Will fix. Maybe it's a good idea to add sysfs fixes into a new patch. I'd prefer a single patch as it fixes the same problem for one structure, the context of use is not that important to justify 2 patches. Agree. I went through the possible uses of superblock again and did not find anything else than the update_super_roots and sysfs read handlers. There are some direct uses of super block members, like label, sys_array, uuid that are passed unconverted and must be accessed via the set/get helpers. AFAIK byte arrays btrfs_super_block::(label, sys_chunk_array, fsid) are not affected by endianness, as memory access for the array starts from the zero in both endianness. And the lowest granularity affected by the endianness is a byte. In some places the superblock is put to a temporary variable so simple grep may miss these. And what about cc this to stable kernel? IIRC it's a very critical problem for btrfs. If the filesystem is always used on a same endianity host, this will not be a problem. Moving between opposite endianity hosts will report bogus numbers in sysfs and the backup root would not be restored correctly. As this is not common, I'd rate thats as a bugfix for stable, but "only" a serious one. agree. Maybe cc: sta...@vger.kernel.org # v3.2+? Thanks for the suggestion. Will do. Any idea what if the patch which applied on mainline ends up conflict on LTS, so write a separate patch to stable? If the patch does not apply to some older stable branch, all involved people get a mail from stable team and have an opportunity to send an updated version of the patch. Regarding the long-term branches, I would consider 4.4 and up. Anything older is a plus but fixing merge conflicts is more likely there so I think the respective maintainers would either fix it by themselves or ask for help. ok. Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On Tue, Feb 13, 2018 at 06:27:13PM +0800, Anand Jain wrote: > On 02/13/2018 05:01 PM, Qu Wenruo wrote: > > On 2018年02月13日 11:00, Anand Jain wrote: > >> Fixes the endianness bug in the fs_info::super_copy by using its > >> btrfs_set_super...() function to set values in the SB, as these > >> functions manage the endianness compatibility nicely. > >> > >> Signed-off-by: Anand Jain > > > > Also went through all btrfs_super_block SETGET functions, greping using > > \>, seems that there are still some left here: > > > > fs/btrfs/sysfs.c: > > In both btrfs_sectorsize_show() and btrfs_clone_alignment_show(): > > return snprintf(buf, PAGE_SIZE, "%u\n", > > fs_info->super_copy->sectorsize); > > > > In btrfs_nodesize_show(): > > return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->nodesize); > > Oh. Thanks. Will fix. Maybe it's a good idea to add sysfs fixes > into a new patch. I'd prefer a single patch as it fixes the same problem for one structure, the context of use is not that important to justify 2 patches. I went through the possible uses of superblock again and did not find anything else than the update_super_roots and sysfs read handlers. There are some direct uses of super block members, like label, sys_array, uuid that are passed unconverted and must be accessed via the set/get helpers. In some places the superblock is put to a temporary variable so simple grep may miss these. > > And what about cc this to stable kernel? > > IIRC it's a very critical problem for btrfs. If the filesystem is always used on a same endianity host, this will not be a problem. Moving between opposite endianity hosts will report bogus numbers in sysfs and the backup root would not be restored correctly. As this is not common, I'd rate thats as a bugfix for stable, but "only" a serious one. > > Maybe cc: sta...@vger.kernel.org # v3.2+? > > Thanks for the suggestion. Will do. Any idea what if the patch which > applied on mainline ends up conflict on LTS, so write a separate patch > to stable? If the patch does not apply to some older stable branch, all involved people get a mail from stable team and have an opportunity to send an updated version of the patch. Regarding the long-term branches, I would consider 4.4 and up. Anything older is a plus but fixing merge conflicts is more likely there so I think the respective maintainers would either fix it by themselves or ask for help. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On 2018年02月13日 18:27, Anand Jain wrote: > > > On 02/13/2018 05:01 PM, Qu Wenruo wrote: >> >> >> On 2018年02月13日 11:00, Anand Jain wrote: >>> Fixes the endianness bug in the fs_info::super_copy by using its >>> btrfs_set_super...() function to set values in the SB, as these >>> functions manage the endianness compatibility nicely. >>> >>> Signed-off-by: Anand Jain >> >> Also went through all btrfs_super_block SETGET functions, greping using >> \>, seems that there are still some left here: >> >> fs/btrfs/sysfs.c: >> In both btrfs_sectorsize_show() and btrfs_clone_alignment_show(): >> return snprintf(buf, PAGE_SIZE, "%u\n", >> fs_info->super_copy->sectorsize); >> >> In btrfs_nodesize_show(): >> return snprintf(buf, PAGE_SIZE, "%u\n", >> fs_info->super_copy->nodesize); > > Oh. Thanks. Will fix. Maybe it's a good idea to add sysfs fixes > into a new patch. > >> And what about cc this to stable kernel? >> IIRC it's a very critical problem for btrfs. >> >> Maybe cc: sta...@vger.kernel.org # v3.2+? > > Thanks for the suggestion. Will do. Any idea what if the patch which > applied on mainline ends up conflict on LTS, so write a separate patch > to stable? In fact I'm not sure about this either. But considering much of the offending code doesn't change since 2009, it wouldn't cause much conflict IIRC. Thanks, Qu > > Thanks, Anand > > >> Thanks, >> Qu >> >>> --- >>> v1->v2: Update change log. Update $Subject. >>> Old: >>> [PATCH] btrfs: use set functions to update latest refs to >>> the SB >>> fs/btrfs/transaction.c | 20 >>> 1 file changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >>> index 04f07144b45c..9220f004001c 100644 >>> --- a/fs/btrfs/transaction.c >>> +++ b/fs/btrfs/transaction.c >>> @@ -1722,19 +1722,23 @@ static void update_super_roots(struct >>> btrfs_fs_info *fs_info) >>> super = fs_info->super_copy; >>> + /* update latest btrfs_super_block::chunk_root refs */ >>> root_item = &fs_info->chunk_root->root_item; >>> - super->chunk_root = root_item->bytenr; >>> - super->chunk_root_generation = root_item->generation; >>> - super->chunk_root_level = root_item->level; >>> + btrfs_set_super_chunk_root(super, root_item->bytenr); >>> + btrfs_set_super_chunk_root_generation(super, >>> root_item->generation); >>> + btrfs_set_super_chunk_root_level(super, root_item->level); >>> + /* update latest btrfs_super_block::root refs */ >>> root_item = &fs_info->tree_root->root_item; >>> - super->root = root_item->bytenr; >>> - super->generation = root_item->generation; >>> - super->root_level = root_item->level; >>> + btrfs_set_super_root(super, root_item->bytenr); >>> + btrfs_set_super_generation(super, root_item->generation); >>> + btrfs_set_super_root_level(super, root_item->level); >>> + >>> if (btrfs_test_opt(fs_info, SPACE_CACHE)) >>> - super->cache_generation = root_item->generation; >>> + btrfs_set_super_cache_generation(super, root_item->generation); >>> if (test_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags)) >>> - super->uuid_tree_generation = root_item->generation; >>> + btrfs_set_super_uuid_tree_generation(super, >>> + root_item->generation); >>> } >>> int btrfs_transaction_in_commit(struct btrfs_fs_info *info) >>> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On 02/13/2018 05:01 PM, Qu Wenruo wrote: On 2018年02月13日 11:00, Anand Jain wrote: Fixes the endianness bug in the fs_info::super_copy by using its btrfs_set_super...() function to set values in the SB, as these functions manage the endianness compatibility nicely. Signed-off-by: Anand Jain Also went through all btrfs_super_block SETGET functions, greping using \>, seems that there are still some left here: fs/btrfs/sysfs.c: In both btrfs_sectorsize_show() and btrfs_clone_alignment_show(): return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->sectorsize); In btrfs_nodesize_show(): return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->nodesize); Oh. Thanks. Will fix. Maybe it's a good idea to add sysfs fixes into a new patch. And what about cc this to stable kernel? IIRC it's a very critical problem for btrfs. Maybe cc: sta...@vger.kernel.org # v3.2+? Thanks for the suggestion. Will do. Any idea what if the patch which applied on mainline ends up conflict on LTS, so write a separate patch to stable? Thanks, Anand Thanks, Qu --- v1->v2: Update change log. Update $Subject. Old: [PATCH] btrfs: use set functions to update latest refs to the SB fs/btrfs/transaction.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 04f07144b45c..9220f004001c 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1722,19 +1722,23 @@ static void update_super_roots(struct btrfs_fs_info *fs_info) super = fs_info->super_copy; + /* update latest btrfs_super_block::chunk_root refs */ root_item = &fs_info->chunk_root->root_item; - super->chunk_root = root_item->bytenr; - super->chunk_root_generation = root_item->generation; - super->chunk_root_level = root_item->level; + btrfs_set_super_chunk_root(super, root_item->bytenr); + btrfs_set_super_chunk_root_generation(super, root_item->generation); + btrfs_set_super_chunk_root_level(super, root_item->level); + /* update latest btrfs_super_block::root refs */ root_item = &fs_info->tree_root->root_item; - super->root = root_item->bytenr; - super->generation = root_item->generation; - super->root_level = root_item->level; + btrfs_set_super_root(super, root_item->bytenr); + btrfs_set_super_generation(super, root_item->generation); + btrfs_set_super_root_level(super, root_item->level); + if (btrfs_test_opt(fs_info, SPACE_CACHE)) - super->cache_generation = root_item->generation; + btrfs_set_super_cache_generation(super, root_item->generation); if (test_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags)) - super->uuid_tree_generation = root_item->generation; + btrfs_set_super_uuid_tree_generation(super, +root_item->generation); } int btrfs_transaction_in_commit(struct btrfs_fs_info *info) -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On 2018年02月13日 11:00, Anand Jain wrote: > Fixes the endianness bug in the fs_info::super_copy by using its > btrfs_set_super...() function to set values in the SB, as these > functions manage the endianness compatibility nicely. > > Signed-off-by: Anand Jain Also went through all btrfs_super_block SETGET functions, greping using \>, seems that there are still some left here: fs/btrfs/sysfs.c: In both btrfs_sectorsize_show() and btrfs_clone_alignment_show(): return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->sectorsize); In btrfs_nodesize_show(): return snprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->nodesize); And what about cc this to stable kernel? IIRC it's a very critical problem for btrfs. Maybe cc: sta...@vger.kernel.org # v3.2+? Thanks, Qu > --- > v1->v2: Update change log. Update $Subject. > Old: > [PATCH] btrfs: use set functions to update latest refs to the SB > fs/btrfs/transaction.c | 20 > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 04f07144b45c..9220f004001c 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1722,19 +1722,23 @@ static void update_super_roots(struct btrfs_fs_info > *fs_info) > > super = fs_info->super_copy; > > + /* update latest btrfs_super_block::chunk_root refs */ > root_item = &fs_info->chunk_root->root_item; > - super->chunk_root = root_item->bytenr; > - super->chunk_root_generation = root_item->generation; > - super->chunk_root_level = root_item->level; > + btrfs_set_super_chunk_root(super, root_item->bytenr); > + btrfs_set_super_chunk_root_generation(super, root_item->generation); > + btrfs_set_super_chunk_root_level(super, root_item->level); > > + /* update latest btrfs_super_block::root refs */ > root_item = &fs_info->tree_root->root_item; > - super->root = root_item->bytenr; > - super->generation = root_item->generation; > - super->root_level = root_item->level; > + btrfs_set_super_root(super, root_item->bytenr); > + btrfs_set_super_generation(super, root_item->generation); > + btrfs_set_super_root_level(super, root_item->level); > + > if (btrfs_test_opt(fs_info, SPACE_CACHE)) > - super->cache_generation = root_item->generation; > + btrfs_set_super_cache_generation(super, root_item->generation); > if (test_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags)) > - super->uuid_tree_generation = root_item->generation; > + btrfs_set_super_uuid_tree_generation(super, > + root_item->generation); > } > > int btrfs_transaction_in_commit(struct btrfs_fs_info *info) > signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] btrfs: fix endianness compatibility during the SB RW
On 13.02.2018 05:00, Anand Jain wrote: > Fixes the endianness bug in the fs_info::super_copy by using its > btrfs_set_super...() function to set values in the SB, as these > functions manage the endianness compatibility nicely. > > Signed-off-by: Anand Jain Reviewed-by: Nikolay Borisov > --- > v1->v2: Update change log. Update $Subject. > Old: > [PATCH] btrfs: use set functions to update latest refs to the SB > fs/btrfs/transaction.c | 20 > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 04f07144b45c..9220f004001c 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1722,19 +1722,23 @@ static void update_super_roots(struct btrfs_fs_info > *fs_info) > > super = fs_info->super_copy; > > + /* update latest btrfs_super_block::chunk_root refs */ > root_item = &fs_info->chunk_root->root_item; > - super->chunk_root = root_item->bytenr; > - super->chunk_root_generation = root_item->generation; > - super->chunk_root_level = root_item->level; > + btrfs_set_super_chunk_root(super, root_item->bytenr); > + btrfs_set_super_chunk_root_generation(super, root_item->generation); > + btrfs_set_super_chunk_root_level(super, root_item->level); > > + /* update latest btrfs_super_block::root refs */ > root_item = &fs_info->tree_root->root_item; > - super->root = root_item->bytenr; > - super->generation = root_item->generation; > - super->root_level = root_item->level; > + btrfs_set_super_root(super, root_item->bytenr); > + btrfs_set_super_generation(super, root_item->generation); > + btrfs_set_super_root_level(super, root_item->level); > + > if (btrfs_test_opt(fs_info, SPACE_CACHE)) > - super->cache_generation = root_item->generation; > + btrfs_set_super_cache_generation(super, root_item->generation); > if (test_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags)) > - super->uuid_tree_generation = root_item->generation; > + btrfs_set_super_uuid_tree_generation(super, > + root_item->generation); > } > > int btrfs_transaction_in_commit(struct btrfs_fs_info *info) > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
