From: Li Nan <[email protected]>
This patch series fixes some simple bugs of setting badblocks and
optimizing struct badblocks. Coly Li has been trying to refactor badblocks
in patch series "badblocks improvement for multiple bad block ranges", but
the workload is significant. Before that, I will fix some easily triggered
issues and optimize some code that does not conflict with Coly's changes.
Changes in v3:
- delete patchs with significant changes.
Li Nan (4):
block/badblocks: change some members of badblocks to bool
block/badblocks: only set bb->changed/unacked_exist when badblocks
changes
block/badblocks: fix badblocks loss when badblocks combine
block/badblocks: fix the bug of reverse order
block/badblocks.c | 38 ++++++++++++++++++++++----------------
include/linux/badblocks.h | 10 +++++-----
2 files changed, 27 insertions(+), 21 deletions(-)
--
2.39.2
From: Li Nan <[email protected]>
Order of badblocks will be reversed if we set a large area at once. 'hi'
remains unchanged while adding continuous badblocks is wrong, the next
setting is greater than 'hi', it should be added to the next position.
Let 'hi' +1 each cycle.
# echo 0 2048 > bad_blocks
# cat bad_blocks
1536 512
1024 512
512 512
0 512
Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <[email protected]>
---
block/badblocks.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/badblocks.c b/block/badblocks.c
index 2c2ef8284a3f..3b816690b940 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -301,6 +301,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
p[hi] = BB_MAKE(s, this_sectors, acknowledged);
sectors -= this_sectors;
s += this_sectors;
+ hi++;
changed = true;
}
}
--
2.39.2
From: Li Nan <[email protected]>
In badblocks_set(), even if no badblocks changes, bb->changed and
unacked_exist will still be set. Only set them when badblocks changes.
Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <[email protected]>
---
block/badblocks.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 1b4caa42c5f1..7e6ebe2ac12c 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -166,6 +166,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
int lo, hi;
int rv = 0;
unsigned long flags;
+ bool changed = false;
if (bb->shift < 0)
/* badblocks are disabled */
@@ -229,6 +230,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
s = a + BB_MAX_LEN;
}
sectors = e - s;
+ changed = true;
}
}
if (sectors && hi < bb->count) {
@@ -259,6 +261,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
sectors = e - s;
lo = hi;
hi++;
+ changed = true;
}
}
if (sectors == 0 && hi < bb->count) {
@@ -277,6 +280,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
memmove(p + hi, p + hi + 1,
(bb->count - hi - 1) * 8);
bb->count--;
+ changed = true;
}
}
while (sectors) {
@@ -299,14 +303,17 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
p[hi] = BB_MAKE(s, this_sectors, acknowledged);
sectors -= this_sectors;
s += this_sectors;
+ changed = true;
}
}
- bb->changed = true;
- if (!acknowledged)
- bb->unacked_exist = true;
- else
- badblocks_update_acked(bb);
+ if (changed) {
+ bb->changed = changed;
+ if (!acknowledged)
+ bb->unacked_exist = true;
+ else
+ badblocks_update_acked(bb);
+ }
write_sequnlock_irqrestore(&bb->lock, flags);
return rv;
--
2.39.2
From: Li Nan <[email protected]>
badblocks will loss if we set it as below:
# echo 1 1 > bad_blocks
# echo 3 1 > bad_blocks
# echo 1 5 > bad_blocks
# cat bad_blocks
1 3
In badblocks_set(), if there is an intersection between p[lo] and p[hi],
we will combine them. The end of new badblocks is p[hi]'s end now. but
p[lo] may cross p[hi] and new end should be the larger of p[lo] and p[hi].
Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Li Nan <[email protected]>
---
block/badblocks.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 7e6ebe2ac12c..2c2ef8284a3f 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -267,16 +267,14 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
if (sectors == 0 && hi < bb->count) {
/* we might be able to combine lo and hi */
/* Note: 's' is at the end of 'lo' */
- sector_t a = BB_OFFSET(p[hi]);
- int lolen = BB_LEN(p[lo]);
- int hilen = BB_LEN(p[hi]);
- int newlen = lolen + hilen - (s - a);
+ sector_t a = BB_OFFSET(p[lo]);
+ int newlen = max(s, BB_OFFSET(p[hi]) + BB_LEN(p[hi])) - a;
- if (s >= a && newlen < BB_MAX_LEN) {
+ if (s >= BB_OFFSET(p[hi]) && newlen < BB_MAX_LEN) {
/* yes, we can combine them */
int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
- p[lo] = BB_MAKE(BB_OFFSET(p[lo]), newlen, ack);
+ p[lo] = BB_MAKE(a, newlen, ack);
memmove(p + hi, p + hi + 1,
(bb->count - hi - 1) * 8);
bb->count--;
--
2.39.2
On Thu, Jun 22, 2023 at 01:20:48AM +0800, [email protected] wrote:
> From: Li Nan <[email protected]>
>
> This patch series fixes some simple bugs of setting badblocks and
> optimizing struct badblocks. Coly Li has been trying to refactor badblocks
> in patch series "badblocks improvement for multiple bad block ranges", but
> the workload is significant. Before that, I will fix some easily triggered
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You mean the refactor is going to take longer to complete?
If so, maybe state it that way...
On Thu, Jun 22, 2023 at 01:20:51AM +0800, [email protected] wrote:
> From: Li Nan <[email protected]>
>
> badblocks will loss if we set it as below:
>
> # echo 1 1 > bad_blocks
> # echo 3 1 > bad_blocks
> # echo 1 5 > bad_blocks
> # cat bad_blocks
> 1 3
>
> In badblocks_set(), if there is an intersection between p[lo] and p[hi],
> we will combine them. The end of new badblocks is p[hi]'s end now. but
> p[lo] may cross p[hi] and new end should be the larger of p[lo] and p[hi].
Reconsider rewriting the commit log. It seems you converted code to
sentence ;-).
Also it might help to show after the patch how the above example would be
for cat bad_blocks
>
> Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
> Signed-off-by: Li Nan <[email protected]>
> ---
> block/badblocks.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 7e6ebe2ac12c..2c2ef8284a3f 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -267,16 +267,14 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> if (sectors == 0 && hi < bb->count) {
> /* we might be able to combine lo and hi */
> /* Note: 's' is at the end of 'lo' */
> - sector_t a = BB_OFFSET(p[hi]);
> - int lolen = BB_LEN(p[lo]);
> - int hilen = BB_LEN(p[hi]);
> - int newlen = lolen + hilen - (s - a);
> + sector_t a = BB_OFFSET(p[lo]);
> + int newlen = max(s, BB_OFFSET(p[hi]) + BB_LEN(p[hi])) - a;
>
> - if (s >= a && newlen < BB_MAX_LEN) {
> + if (s >= BB_OFFSET(p[hi]) && newlen < BB_MAX_LEN) {
> /* yes, we can combine them */
> int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
>
> - p[lo] = BB_MAKE(BB_OFFSET(p[lo]), newlen, ack);
> + p[lo] = BB_MAKE(a, newlen, ack);
> memmove(p + hi, p + hi + 1,
> (bb->count - hi - 1) * 8);
> bb->count--;
> --
> 2.39.2
>
On Thu, Jun 22, 2023 at 01:20:52AM +0800, [email protected] wrote:
> From: Li Nan <[email protected]>
>
> Order of badblocks will be reversed if we set a large area at once. 'hi'
> remains unchanged while adding continuous badblocks is wrong, the next
> setting is greater than 'hi', it should be added to the next position.
> Let 'hi' +1 each cycle.
The commitlog needs more work.
>
> # echo 0 2048 > bad_blocks
> # cat bad_blocks
> 1536 512
> 1024 512
> 512 512
> 0 512
Is the above before or after this patch is applied?
>
> Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
> Signed-off-by: Li Nan <[email protected]>
> ---
> block/badblocks.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 2c2ef8284a3f..3b816690b940 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -301,6 +301,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> p[hi] = BB_MAKE(s, this_sectors, acknowledged);
> sectors -= this_sectors;
> s += this_sectors;
> + hi++;
> changed = true;
> }
> }
> --
> 2.39.2
>
在 2023/6/21 21:32, Ashok Raj 写道:
> On Thu, Jun 22, 2023 at 01:20:48AM +0800, [email protected] wrote:
>> From: Li Nan <[email protected]>
>>
>> This patch series fixes some simple bugs of setting badblocks and
>> optimizing struct badblocks. Coly Li has been trying to refactor badblocks
>> in patch series "badblocks improvement for multiple bad block ranges", but
>> the workload is significant. Before that, I will fix some easily triggered
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> You mean the refactor is going to take longer to complete?
Yes, refer to the link:
https://lore.kernel.org/all/[email protected]
> If so, maybe state it that way...
>
> .
--
Thanks,
Nan
在 2023/6/21 22:09, Ashok Raj 写道:
> On Thu, Jun 22, 2023 at 01:20:51AM +0800, [email protected] wrote:
>> From: Li Nan <[email protected]>
>>
>> badblocks will loss if we set it as below:
>>
>> # echo 1 1 > bad_blocks
>> # echo 3 1 > bad_blocks
>> # echo 1 5 > bad_blocks
>> # cat bad_blocks
>> 1 3
>>
>> In badblocks_set(), if there is an intersection between p[lo] and p[hi],
>> we will combine them. The end of new badblocks is p[hi]'s end now. but
>> p[lo] may cross p[hi] and new end should be the larger of p[lo] and p[hi].
>
> Reconsider rewriting the commit log. It seems you converted code to
> sentence ;-).
I will rewrite log.
>
> Also it might help to show after the patch how the above example would be
> for cat bad_blocks
>
after patch:
# cat bad_blocks
1 5
I will show it in next version. Thanks for your suggestion.
>>
>> Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
>> Signed-off-by: Li Nan <[email protected]>
>> ---
>> block/badblocks.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/badblocks.c b/block/badblocks.c
>> index 7e6ebe2ac12c..2c2ef8284a3f 100644
>> --- a/block/badblocks.c
>> +++ b/block/badblocks.c
>> @@ -267,16 +267,14 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>> if (sectors == 0 && hi < bb->count) {
>> /* we might be able to combine lo and hi */
>> /* Note: 's' is at the end of 'lo' */
>> - sector_t a = BB_OFFSET(p[hi]);
>> - int lolen = BB_LEN(p[lo]);
>> - int hilen = BB_LEN(p[hi]);
>> - int newlen = lolen + hilen - (s - a);
>> + sector_t a = BB_OFFSET(p[lo]);
>> + int newlen = max(s, BB_OFFSET(p[hi]) + BB_LEN(p[hi])) - a;
>>
>> - if (s >= a && newlen < BB_MAX_LEN) {
>> + if (s >= BB_OFFSET(p[hi]) && newlen < BB_MAX_LEN) {
>> /* yes, we can combine them */
>> int ack = BB_ACK(p[lo]) && BB_ACK(p[hi]);
>>
>> - p[lo] = BB_MAKE(BB_OFFSET(p[lo]), newlen, ack);
>> + p[lo] = BB_MAKE(a, newlen, ack);
>> memmove(p + hi, p + hi + 1,
>> (bb->count - hi - 1) * 8);
>> bb->count--;
>> --
>> 2.39.2
>>
>
> .
--
Thanks,
Nan
在 2023/6/21 22:15, Ashok Raj 写道:
> On Thu, Jun 22, 2023 at 01:20:52AM +0800, [email protected] wrote:
>> From: Li Nan <[email protected]>
>>
>> Order of badblocks will be reversed if we set a large area at once. 'hi'
>> remains unchanged while adding continuous badblocks is wrong, the next
>> setting is greater than 'hi', it should be added to the next position.
>> Let 'hi' +1 each cycle.
>
> The commitlog needs more work.
OK, I will improve this.
>>
>> # echo 0 2048 > bad_blocks
>> # cat bad_blocks
>> 1536 512
>> 1024 512
>> 512 512
>> 0 512
>
> Is the above before or after this patch is applied?
All badblocks are arranged from small to large. after patch:
# cat bad_blocks
0 512
512 512
1024 512
1536 512
I will show it in next version. Thanks for your suggestion.
>
>>
>> Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
>> Signed-off-by: Li Nan <[email protected]>
>> ---
>> block/badblocks.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/block/badblocks.c b/block/badblocks.c
>> index 2c2ef8284a3f..3b816690b940 100644
>> --- a/block/badblocks.c
>> +++ b/block/badblocks.c
>> @@ -301,6 +301,7 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>> p[hi] = BB_MAKE(s, this_sectors, acknowledged);
>> sectors -= this_sectors;
>> s += this_sectors;
>> + hi++;
>> changed = true;
>> }
>> }
>> --
>> 2.39.2
>>
>
> .
--
Thanks,
Nan