On 6/18/19 10:39 PM, Song Liu wrote:
On Tue, Jun 18, 2019 at 1:35 AM Guoqing Jiang <gqjiang@xxxxxxxx> wrote:
On 6/18/19 11:41 AM, Guoqing Jiang wrote:
+static int check_and_add_wb(struct md_rdev *rdev, sector_t lo,
sector_t hi)
+{
+ struct wb_info *wi;
+ unsigned long flags;
+ int ret = 0;
+ struct mddev *mddev = rdev->mddev;
+
+ wi = mempool_alloc(mddev->wb_info_pool, GFP_NOIO);
+
+ spin_lock_irqsave(&rdev->wb_list_lock, flags);
+ list_for_each_entry(wi, &rdev->wb_list, list) {
This doesn't look right. We should allocate wi from mempool after
the list_for_each_entry(), right?
Good catch, I will use an temp variable in the iteration since
mempool_alloc
could sleep.
After a second thought, I think it should be fine since wi is either
reused to record the sector range or freed after the iteration.
Could you elaborate your concern? Thanks.
First, we allocated wb_info and stored the ptr to wi. Then we use
same wi in list_for_each_entry(), where the original value of wi is
overwritten:
#define list_for_each_entry(pos, head, member) \
for (pos = list_first_entry(head, typeof(*pos), member); \
&pos->member != (head); \
pos = list_next_entry(pos, member))
So we are leaking the original wi from mempool_alloc, right?
Yes, you are right, thanks for the details.
And we probably over writing one element on wb_list.
Will change it to:
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 93dff41c4ff9..f1ea17b14b6e 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -85,7 +85,7 @@ static void lower_barrier(struct r1conf *conf,
sector_t sector_nr);
static int check_and_add_wb(struct md_rdev *rdev, sector_t lo,
sector_t hi)
{
- struct wb_info *wi;
+ struct wb_info *wi, *temp_wi;
unsigned long flags;
int ret = 0;
struct mddev *mddev = rdev->mddev;
@@ -93,9 +93,9 @@ static int check_and_add_wb(struct md_rdev *rdev,
sector_t lo, sector_t hi)
wi = mempool_alloc(mddev->wb_info_pool, GFP_NOIO);
spin_lock_irqsave(&rdev->wb_list_lock, flags);
- list_for_each_entry(wi, &rdev->wb_list, list) {
+ list_for_each_entry(temp_wi, &rdev->wb_list, list) {
/* collision happened */
- if (hi > wi->lo && lo < wi->hi) {
+ if (hi > temp_wi->lo && lo < temp_wi->hi) {
ret = -EBUSY;
break;
}
BRs,
Guoqing
