Message ID | 1527594317-9214-1-git-send-email-wshilong1991@gmail.com |
---|---|
State | Accepted, archived |
Headers | show |
Series | [1/5] ext4: fix race with setting free_inode/clusters_counter | expand |
On May 29, 2018, at 5:45 AM, Wang Shilong <wangshilong1991@gmail.com> wrote: > > From: Wang Shilong <wshilong@ddn.com> > > Whenever we hit block or inode bitmap corruptions we set > bit and then reduce this block group free inode/clusters > counter to expose right available space. > > However some of ext4_mark_group_bitmap_corrupted() is called > inside group spinlock, some are not, this could make it happen > that we double reduce one block group free counters from system. > > Always hold group spinlock for it could fix it, but it looks > a little heavy, we could use test_and_set_bit() to fix race > problems here. > > Signed-off-by: Wang Shilong <wshilong@ddn.com> Reviewed-by: Andreas Dilger <adilger@dilger.ca> > --- > fs/ext4/super.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index c1c5c87..d6fa6cf 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -770,26 +770,26 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, > struct ext4_sb_info *sbi = EXT4_SB(sb); > struct ext4_group_info *grp = ext4_get_group_info(sb, group); > struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); > + int ret; > > - if ((flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) && > - !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) { > - percpu_counter_sub(&sbi->s_freeclusters_counter, > - grp->bb_free); > - set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, > - &grp->bb_state); > + if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { > + ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, > + &grp->bb_state); > + if (!ret) > + percpu_counter_sub(&sbi->s_freeclusters_counter, > + grp->bb_free); > } > > - if ((flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) && > - !EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) { > - if (gdp) { > + if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { > + ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, > + &grp->bb_state); > + if (!ret && gdp) { > int count; > > count = ext4_free_inodes_count(sb, gdp); > percpu_counter_sub(&sbi->s_freeinodes_counter, > count); > } > - set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, > - &grp->bb_state); > } > } > > -- > 1.8.3.1 > Cheers, Andreas
Hi Ted, Would you please consider this patchset for new merge windows? They are all reviewed by Andreas at least. Thanks, Shilong On Tue, Jun 5, 2018 at 2:00 AM, Andreas Dilger <adilger@dilger.ca> wrote: > On May 29, 2018, at 5:45 AM, Wang Shilong <wangshilong1991@gmail.com> > wrote: > > > > From: Wang Shilong <wshilong@ddn.com> > > > > Whenever we hit block or inode bitmap corruptions we set > > bit and then reduce this block group free inode/clusters > > counter to expose right available space. > > > > However some of ext4_mark_group_bitmap_corrupted() is called > > inside group spinlock, some are not, this could make it happen > > that we double reduce one block group free counters from system. > > > > Always hold group spinlock for it could fix it, but it looks > > a little heavy, we could use test_and_set_bit() to fix race > > problems here. > > > > Signed-off-by: Wang Shilong <wshilong@ddn.com> > > Reviewed-by: Andreas Dilger <adilger@dilger.ca> > > > --- > > fs/ext4/super.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index c1c5c87..d6fa6cf 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -770,26 +770,26 @@ void ext4_mark_group_bitmap_corrupted(struct > super_block *sb, > > struct ext4_sb_info *sbi = EXT4_SB(sb); > > struct ext4_group_info *grp = ext4_get_group_info(sb, group); > > struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); > > + int ret; > > > > - if ((flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) && > > - !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) { > > - percpu_counter_sub(&sbi->s_freeclusters_counter, > > - grp->bb_free); > > - set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, > > - &grp->bb_state); > > + if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { > > + ret = ext4_test_and_set_bit(EXT4_ > GROUP_INFO_BBITMAP_CORRUPT_BIT, > > + &grp->bb_state); > > + if (!ret) > > + percpu_counter_sub(&sbi->s_freeclusters_counter, > > + grp->bb_free); > > } > > > > - if ((flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) && > > - !EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) { > > - if (gdp) { > > + if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { > > + ret = ext4_test_and_set_bit(EXT4_ > GROUP_INFO_IBITMAP_CORRUPT_BIT, > > + &grp->bb_state); > > + if (!ret && gdp) { > > int count; > > > > count = ext4_free_inodes_count(sb, gdp); > > percpu_counter_sub(&sbi->s_freeinodes_counter, > > count); > > } > > - set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, > > - &grp->bb_state); > > } > > } > > > > -- > > 1.8.3.1 > > > > > Cheers, Andreas > > > > > > <div dir="ltr"><div class="gmail_extra">Hi Ted,</div><div class="gmail_extra"><br></div><div class="gmail_extra">Would you please consider this patchset for new merge windows?</div><div class="gmail_extra">They are all reviewed by Andreas at least.</div><div class="gmail_extra"><div class="gmail_quote"><br></div><div class="gmail_quote">Thanks,</div><div class="gmail_quote">Shilong</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Tue, Jun 5, 2018 at 2:00 AM, Andreas Dilger <span dir="ltr"><<a href="mailto:adilger@dilger.ca" target="_blank">adilger@dilger.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On May 29, 2018, at 5:45 AM, Wang Shilong <<a href="mailto:wangshilong1991@gmail.com">wangshilong1991@gmail.com</a>> wrote:<br> > <br> > From: Wang Shilong <<a href="mailto:wshilong@ddn.com">wshilong@ddn.com</a>><br> > <br> > Whenever we hit block or inode bitmap corruptions we set<br> > bit and then reduce this block group free inode/clusters<br> > counter to expose right available space.<br> > <br> > However some of ext4_mark_group_bitmap_<wbr>corrupted() is called<br> > inside group spinlock, some are not, this could make it happen<br> > that we double reduce one block group free counters from system.<br> > <br> > Always hold group spinlock for it could fix it, but it looks<br> > a little heavy, we could use test_and_set_bit() to fix race<br> > problems here.<br> > <br> > Signed-off-by: Wang Shilong <<a href="mailto:wshilong@ddn.com">wshilong@ddn.com</a>><br> <br> Reviewed-by: Andreas Dilger <<a href="mailto:adilger@dilger.ca">adilger@dilger.ca</a>><br> <br> > ---<br> > fs/ext4/super.c | 22 +++++++++++-----------<br> > 1 file changed, 11 insertions(+), 11 deletions(-)<br> > <br> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c<br> > index c1c5c87..d6fa6cf 100644<br> > --- a/fs/ext4/super.c<br> > +++ b/fs/ext4/super.c<br> > @@ -770,26 +770,26 @@ void ext4_mark_group_bitmap_<wbr>corrupted(struct super_block *sb,<br> > struct ext4_sb_info *sbi = EXT4_SB(sb);<br> > struct ext4_group_info *grp = ext4_get_group_info(sb, group);<br> > struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL);<br> > + int ret;<br> > <br> > - if ((flags & EXT4_GROUP_INFO_BBITMAP_<wbr>CORRUPT) &&<br> > - !EXT4_MB_GRP_BBITMAP_CORRUPT(<wbr>grp)) {<br> > - percpu_counter_sub(&sbi->s_<wbr>freeclusters_counter,<br> > - grp->bb_free);<br> > - set_bit(EXT4_GROUP_INFO_<wbr>BBITMAP_CORRUPT_BIT,<br> > - &grp->bb_state);<br> > + if (flags & EXT4_GROUP_INFO_BBITMAP_<wbr>CORRUPT) {<br> > + ret = ext4_test_and_set_bit(EXT4_<wbr>GROUP_INFO_BBITMAP_CORRUPT_<wbr>BIT,<br> > + &grp->bb_state);<br> > + if (!ret)<br> > + percpu_counter_sub(&sbi->s_<wbr>freeclusters_counter,<br> > + grp->bb_free);<br> > }<br> > <br> > - if ((flags & EXT4_GROUP_INFO_IBITMAP_<wbr>CORRUPT) &&<br> > - !EXT4_MB_GRP_IBITMAP_CORRUPT(<wbr>grp)) {<br> > - if (gdp) {<br> > + if (flags & EXT4_GROUP_INFO_IBITMAP_<wbr>CORRUPT) {<br> > + ret = ext4_test_and_set_bit(EXT4_<wbr>GROUP_INFO_IBITMAP_CORRUPT_<wbr>BIT,<br> > + &grp->bb_state);<br> > + if (!ret && gdp) {<br> > int count;<br> > <br> > count = ext4_free_inodes_count(sb, gdp);<br> > percpu_counter_sub(&sbi->s_<wbr>freeinodes_counter,<br> > count);<br> > }<br> > - set_bit(EXT4_GROUP_INFO_<wbr>IBITMAP_CORRUPT_BIT,<br> > - &grp->bb_state);<br> > }<br> > }<br> > <br> > --<br> > 1.8.3.1<br> > <br> <br> <br> Cheers, Andreas<br> <br> <br> <br> <br> <br> </blockquote></div><br></div></div>
On Wed, Jul 25, 2018 at 08:38:19AM +0800, Wang Shilong wrote: > Hi Ted, > > Would you please consider this patchset for new merge windows? > They are all reviewed by Andreas at least. The commit descriptions don't explain *why* things are changing, and in many cases you are doing lots of refactoring that is hard to validate. So I've been putting off this patch. Andreas tried to tell me more of the context of what is going on, so it's on my list. But patches that are easy to review and understang get processed first. - Ted
On Tue, May 29, 2018 at 08:45:13PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > Whenever we hit block or inode bitmap corruptions we set > bit and then reduce this block group free inode/clusters > counter to expose right available space. > > However some of ext4_mark_group_bitmap_corrupted() is called > inside group spinlock, some are not, this could make it happen > that we double reduce one block group free counters from system. > > Always hold group spinlock for it could fix it, but it looks > a little heavy, we could use test_and_set_bit() to fix race > problems here. > > Signed-off-by: Wang Shilong <wshilong@ddn.com> Applied, thanks. - Ted
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index c1c5c87..d6fa6cf 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -770,26 +770,26 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, struct ext4_sb_info *sbi = EXT4_SB(sb); struct ext4_group_info *grp = ext4_get_group_info(sb, group); struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); + int ret; - if ((flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) && - !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) { - percpu_counter_sub(&sbi->s_freeclusters_counter, - grp->bb_free); - set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, - &grp->bb_state); + if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { + ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, + &grp->bb_state); + if (!ret) + percpu_counter_sub(&sbi->s_freeclusters_counter, + grp->bb_free); } - if ((flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) && - !EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) { - if (gdp) { + if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { + ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, + &grp->bb_state); + if (!ret && gdp) { int count; count = ext4_free_inodes_count(sb, gdp); percpu_counter_sub(&sbi->s_freeinodes_counter, count); } - set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, - &grp->bb_state); } }