| Message ID | 20190417141918.10648-1-ntsironis@arrikto.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm cache metadata: Fix loading discard bitset | expand |
On Wed, Apr 17 2019 at 10:19am -0400, Nikos Tsironis <ntsironis@arrikto.com> wrote: > Add missing dm_bitset_cursor_next() to properly advance the bitset > cursor. > > Otherwise, the discarded state of all blocks is set according to the > discarded state of the first block. > > Fixes: ae4a46a1f6 ("dm cache metadata: use bitset cursor api to load discard bitset") > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> Nice catch, I'll obviously mark this for stable@ too. One comment below. > --- > drivers/md/dm-cache-metadata.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c > index 6fc93834da44..151aa95775be 100644 > --- a/drivers/md/dm-cache-metadata.c > +++ b/drivers/md/dm-cache-metadata.c > @@ -1167,11 +1167,18 @@ static int __load_discards(struct dm_cache_metadata *cmd, > if (r) > return r; > > - for (b = 0; b < from_dblock(cmd->discard_nr_blocks); b++) { > + for (b = 0; ; b++) { > r = fn(context, cmd->discard_block_size, to_dblock(b), > dm_bitset_cursor_get_value(&c)); > if (r) > break; > + > + if (b >= (from_dblock(cmd->discard_nr_blocks) - 1)) > + break; Not understanding why you moved the conditional inside the loop, I'd rather keep it in the for(). Or did you have some reason for this? > + > + r = dm_bitset_cursor_next(&c); > + if (r) > + break; > } > > dm_bitset_cursor_end(&c); > -- > 2.11.0 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 4/17/19 5:35 PM, Mike Snitzer wrote: > On Wed, Apr 17 2019 at 10:19am -0400, > Nikos Tsironis <ntsironis@arrikto.com> wrote: > >> Add missing dm_bitset_cursor_next() to properly advance the bitset >> cursor. >> >> Otherwise, the discarded state of all blocks is set according to the >> discarded state of the first block. >> >> Fixes: ae4a46a1f6 ("dm cache metadata: use bitset cursor api to load discard bitset") >> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> > > Nice catch, I'll obviously mark this for stable@ too. > > One comment below. > >> --- >> drivers/md/dm-cache-metadata.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c >> index 6fc93834da44..151aa95775be 100644 >> --- a/drivers/md/dm-cache-metadata.c >> +++ b/drivers/md/dm-cache-metadata.c >> @@ -1167,11 +1167,18 @@ static int __load_discards(struct dm_cache_metadata *cmd, >> if (r) >> return r; >> >> - for (b = 0; b < from_dblock(cmd->discard_nr_blocks); b++) { >> + for (b = 0; ; b++) { >> r = fn(context, cmd->discard_block_size, to_dblock(b), >> dm_bitset_cursor_get_value(&c)); >> if (r) >> break; >> + >> + if (b >= (from_dblock(cmd->discard_nr_blocks) - 1)) >> + break; > > Not understanding why you moved the conditional inside the loop, I'd > rather keep it in the for(). Or did you have some reason for this? > After processing the last block, i.e., b == (from_dblock(cmd->discard_nr_blocks) - 1), dm_bitset_cursor_next() will return -ENODATA. It seemed to me simpler to move the conditional inside the loop, rather than check explicitly for -ENODATA and handle it differently, e.g., set r = 0, if r == -ENODATA. Nikos. >> + >> + r = dm_bitset_cursor_next(&c); >> + if (r) >> + break; >> } >> >> dm_bitset_cursor_end(&c); >> -- >> 2.11.0 >> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c index 6fc93834da44..151aa95775be 100644 --- a/drivers/md/dm-cache-metadata.c +++ b/drivers/md/dm-cache-metadata.c @@ -1167,11 +1167,18 @@ static int __load_discards(struct dm_cache_metadata *cmd, if (r) return r; - for (b = 0; b < from_dblock(cmd->discard_nr_blocks); b++) { + for (b = 0; ; b++) { r = fn(context, cmd->discard_block_size, to_dblock(b), dm_bitset_cursor_get_value(&c)); if (r) break; + + if (b >= (from_dblock(cmd->discard_nr_blocks) - 1)) + break; + + r = dm_bitset_cursor_next(&c); + if (r) + break; } dm_bitset_cursor_end(&c);
Add missing dm_bitset_cursor_next() to properly advance the bitset cursor. Otherwise, the discarded state of all blocks is set according to the discarded state of the first block. Fixes: ae4a46a1f6 ("dm cache metadata: use bitset cursor api to load discard bitset") Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com> --- drivers/md/dm-cache-metadata.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)