Don't merge different partition's IOs

The problem is caused by merging different partition's I/Os. So the patch check whether a merging bio or request is a same partition as a request or not by using a partition's start sector and size.

2010/12/6 Yasuaki Ishimatsu<[email protected]>:

Quoted 4 lines

The problem is caused by merging different partition's I/Os. So the patch check whether a merging bio or request is a same partition as a request or not by using a partition's start sector and size.

I really think this is wrong.

We should just carry the partition information around in the req and the bio, and just compare the pointers, rather than compare the range. No need to even dereference the pointers, you should be able to just do

    /* don't merge if not on the same partition */
    if (bio->part != req->part)
       return 0;

or something.

This is doubly true since the accounting already does that horrible partition lookup: rather than look it up, we should just _set_ it in __generic_make_request(), where I think we already know it since we do that whole blk_partition_remap().

So just something like the appended (TOTALLY UNTESTED) perhaps?

Note that this should get it right even for overlapping partitions etc.

Linus

Hi Linus, Yasuaki, and Jens

Quoted 24 lines · Linus Torvalds

2010/12/6 Yasuaki Ishimatsu<[email protected]>:

Quoted 4 lines

The problem is caused by merging different partition's I/Os. So the patch check whether a merging bio or request is a same partition as a request or not by using a partition's start sector and size.

I really think this is wrong.

We should just carry the partition information around in the req and the bio, and just compare the pointers, rather than compare the range. No need to even dereference the pointers, you should be able to just do

   /* don't merge if not on the same partition */
   if (bio->part != req->part)
      return 0;

or something.

This is doubly true since the accounting already does that horrible partition lookup: rather than look it up, we should just _set_ it in __generic_make_request(), where I think we already know it since we do that whole blk_partition_remap().

Quoted 5 lines

So just something like the appended (TOTALLY UNTESTED) perhaps?

Note that this should get it right even for overlapping partitions etc.

Linus

The problem can occur even if your patches are applied. Think about a case like the following.

1) There are 2 partition, sda1 and sda2, on sda.
2) Open sda and issue an IO to sda2's first sector. Then sda2's in_flight is incremented though you open not sda2 but sda. It is because of partition lookup method. It is based on which partition rq->__sector sector belongs to.
3) Issue an IO to sda1's last sector and it merged to the IO issued in step (2) because their part are both sda. In addition, rq->__sector is modified to the sda1's region.
4) After completing the IO, sda1's in_flight is decremented and diskstat is corrupted here.

I think fixing this case is difficult and would cause more complexity.

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

- It can fix the problem which Yasuaki reported, including the cases which I mentioned above.
- It only append one extra field to request.

Although it would causes a bit gap, it doesn't have most influences because merging requests beyond partitions is the rare case.

I confirmed the attached patch can be applied to 2.6.37-rc4 and succeeded to compile. If you can accept this idea, I'll test it soon.

---
 block/blk-core.c       |   12 +++++++-----
 block/blk-merge.c      |    2 +-
 include/linux/blkdev.h |    6 ++++++
 3 files changed, 14 insertions(+), 6 deletions(-)

Index: linux-2.6.37-rc4/block/blk-core.c
===================================================================

block/blk-core.c +0 -1

@@ -64,11 +64,13 @@ static void drive_stat_acct(struct reque
 		return;
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

block/blk-core.c +2 -2

 	}

@@ -1776,7 +1778,7 @@ static void blk_account_io_completion(st
 		int cpu;
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = disk_map_sector_rcu(req->rq_disk, blk_rq_init_pos(req));
 		part_stat_add(cpu, part, sectors[rw], bytes >> 9);
 		part_stat_unlock();
 	}

@@ -1796,7 +1798,7 @@ static void blk_account_io_done(struct r
 		int cpu;
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = disk_map_sector_rcu(req->rq_disk, blk_rq_init_pos(req));
 		part_stat_inc(cpu, part, ios[rw]);
 		part_stat_add(cpu, part, ticks[rw], duration);

Index: linux-2.6.37-rc4/block/blk-merge.c
===================================================================

block/blk-merge.c +1 -1

@@ -351,7 +351,7 @@ static void blk_account_io_merge(struct
 		int cpu;
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = disk_map_sector_rcu(req->rq_disk, blk_rq_init_pos(req));
 		part_round_stats(cpu, part);
 		part_dec_in_flight(part, rq_data_dir(req));

Index: linux-2.6.37-rc4/include/linux/blkdev.h
===================================================================

Hi Linus, Yasuaki, and Jens

Quoted 24 lines · Linus Torvalds

2010/12/6 Yasuaki Ishimatsu<[email protected]>:

Quoted 4 lines

The problem is caused by merging different partition's I/Os. So the patch check whether a merging bio or request is a same partition as a request or not by using a partition's start sector and size.

I really think this is wrong.

We should just carry the partition information around in the req and the bio, and just compare the pointers, rather than compare the range. No need to even dereference the pointers, you should be able to just do

    /* don't merge if not on the same partition */
    if (bio->part != req->part)
       return 0;

or something.

This is doubly true since the accounting already does that horrible partition lookup: rather than look it up, we should just _set_ it in __generic_make_request(), where I think we already know it since we do that whole blk_partition_remap().

Quoted 5 lines

So just something like the appended (TOTALLY UNTESTED) perhaps?

Note that this should get it right even for overlapping partitions etc.

Linus

The problem can occur even if your patches are applied. Think about a case like the following.

1) There are 2 partition, sda1 and sda2, on sda.
2) Open sda and issue an IO to sda2's first sector. Then sda2's in_flight is incremented though you open not sda2 but sda. It is because of partition lookup method. It is based on which partition rq->__sector sector belongs to.
3) Issue an IO to sda1's last sector and it merged to the IO issued in step (2) because their part are both sda. In addition, rq->__sector is modified to the sda1's region.
4) After completing the IO, sda1's in_flight is decremented and diskstat is corrupted here.

I think fixing this case is difficult and would cause more complexity.

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

Quoted 4 lines · Satoru Takeuchi

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

I really would prefer if we fixed up the patchset we ended up reverting. At least that had a purpose with growing struct request, since we saved on doing the partition lookups.

Hi Jens,

Quoted 6 lines · Jens Axboe

Quoted 4 lines · Satoru Takeuchi

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

Revert is already finished. 2.6.37-rc-5 and latest stable kernel doesn't contain Yasuaki's former logic.

https://lkml.org/lkml/2010/10/24/118

Quoted 3 lines

I really would prefer if we fixed up the patchset we ended up reverting. At least that had a purpose with growing struct request, since we saved on doing the partition lookups.

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

Revert is already finished. 2.6.37-rc-5 and latest stable kernel doesn't contain Yasuaki's former logic.

https://lkml.org/lkml/2010/10/24/118

Quoted 3 lines · that is why I

Quoted 3 lines

I really would prefer if we fixed up the patchset we ended up reverting. At least that had a purpose with growing struct request, since we saved on doing the partition lookups.

That I prefer we fix that code up, since I think it's the best solution to the problem.

Hi Jens,

Quoted 10 lines · Jens Axboe

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

Revert is already finished. 2.6.37-rc-5 and latest stable kernel doesn't contain Yasuaki's former logic.

https://lkml.org/lkml/2010/10/24/118

Quoted 9 lines

Quoted 3 lines · that is why I

Quoted 3 lines

I really would prefer if we fixed up the patchset we ended up reverting.. At least that had a purpose with growing struct request, since we saved on doing the partition lookups.

That I prefer we fix that code up, since I think it's the best solution to the problem.

I already postedit.

https://lkml.org/lkml/2010/12/8/12

I think it is OK without mail subject :-)

Quoted 27 lines · Satoru Takeuchi

Hi Jens,

Quoted 10 lines · Jens Axboe

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

Revert is already finished. 2.6.37-rc-5 and latest stable kernel doesn't contain Yasuaki's former logic.

https://lkml.org/lkml/2010/10/24/118

Quoted 9 lines

Quoted 3 lines · that is why I

Quoted 3 lines

I really would prefer if we fixed up the patchset we ended up reverting.. At least that had a purpose with growing struct request, since we saved on doing the partition lookups.

That I prefer we fix that code up, since I think it's the best solution to the problem.

I already postedit.

https://lkml.org/lkml/2010/12/8/12

Quoted 1 line

I think it is OK without mail subject :-)

No, that's not it at all. What I mean (and what was reverted) was caching the partition lookup, and using that for the stats. The problem with that approach turned out to be the elevator queiscing logic not being fully correct. One easier way to fix that would be to reference count the part stats, instead of having to drain the queue.

Quoted 28 lines · Jens Axboe

Quoted 27 lines · Satoru Takeuchi

Hi Jens,

Quoted 10 lines · Jens Axboe

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

I hit on another approach. Although it doesn'tprevent any merge as Linus preferred, it can fix the problem anyway. In this idea, in_flight is incremented and decremented for the partition which the request belonged to in its creation. It has the following merits.

Revert is already finished. 2.6.37-rc-5 and latest stable kernel doesn't contain Yasuaki's former logic.

https://lkml.org/lkml/2010/10/24/118

Quoted 9 lines

Quoted 3 lines · that is why I

Quoted 3 lines

I really would prefer if we fixed up the patchset we ended up reverting.. At least that had a purpose with growing struct request, since we saved on doing the partition lookups.

That I prefer we fix that code up, since I think it's the best solution to the problem.

I already postedit.

https://lkml.org/lkml/2010/12/8/12

Quoted 7 lines

Quoted 1 line

I think it is OK without mail subject :-)

No, that's not it at all. What I mean (and what was reverted) was caching the partition lookup, and using that for the stats. The problem with that approach turned out to be the elevator queiscing logic not being fully correct. One easier way to fix that would be to reference count the part stats, instead of having to drain the queue.

Taking reference to hd_struct and storing it in rq, will definitely save us 1 lookup while doing accounting on completion path. It does not save on rq size though.

IIUC, current patch does not increase the number of existing lookups. So current situation does not deteriorate with the patch.

But storing a reference in rq and avoiding 1 lookup in completion path definitely sounds better.

Storing a pointer to partition in rq also got the advantage that we can easily not allow merging of requests across partitions for better accounting.

Satoru, so yes, if you can implement what jens is suggesting, would be good.

Thanks
Vivek

No, that's not it at all. What I mean (and what was reverted) was caching the partition lookup, and using that for the stats. The problem with that approach turned out to be the elevator queiscing logic not being fully correct. One easier way to fix that would be to reference count the part stats, instead of having to drain the queue.

Quoted 7 lines · Jens Axboe

No, that's not it at all. What I mean (and what was reverted) was caching the partition lookup, and using that for the stats. The problem with that approach turned out to be the elevator queiscing logic not being fully correct. One easier way to fix that would be to reference count the part stats, instead of having to drain the queue.

The partition is already deleted in a rcu callback function. If I understand RCU correctly, we just need to use rcu_dereference() each time we use rq->part. Something like the following *untested* patch.

Jerome

---

block/blk-core.c +8 -5

index 4ce953f..70d23f9 100644
@@ -64,13 +64,15 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 
-	if (!new_io)
+	if (!new_io) {
+		part = blk_rq_get_part(rq);
 		part_stat_inc(cpu, part, merges[rw]);
-	else {
+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 		part_round_stats(cpu, part);
 		part_inc_in_flight(part, rw);
+		rq->part = part;
 	}
 
 	part_stat_unlock();

@@ -128,6 +130,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->ref_count = 1;
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
+	rq->part = NULL;
 }
 EXPORT_SYMBOL(blk_rq_init);
 

@@ -1776,7 +1784,7 @@ static void blk_account_io_completion(struct request *req, unsigned int bytes)
 		int cpu;
 
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = blk_rq_get_part(req);
 		part_stat_add(cpu, part, sectors[rw], bytes >> 9);
 		part_stat_unlock();
 	}

@@ -1796,7 +1804,7 @@ static void blk_account_io_done(struct request *req)
 		int cpu;
 
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = blk_rq_get_part(req);
 
 		part_stat_inc(cpu, part, ios[rw]);
 		part_stat_add(cpu, part, ticks[rw], duration);

block/blk-merge.c +1 -1

index 77b7c26..0ea5416 100644
@@ -351,7 +351,7 @@ static void blk_account_io_merge(struct request *req)
 		int cpu;
 
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = blk_rq_get_part(req);
 
 		part_round_stats(cpu, part);
 		part_dec_in_flight(part, rq_data_dir(req));

include/linux/blkdev.h +7 -0

index aae86fd..00a3b93 100644
@@ -115,6 +115,7 @@ struct request {
 	void *elevator_private3;
 
 	struct gendisk *rq_disk;
+	struct hd_struct __rcu *part;
 	unsigned long start_time;
 #ifdef CONFIG_BLK_CGROUP
 	unsigned long long start_time_ns;

@@ -752,6 +753,12 @@ static inline unsigned int blk_rq_cur_sectors(const struct request *rq)
 	return blk_rq_cur_bytes(rq) >> 9;
 }
 
+static inline struct hd_struct *blk_rq_get_part(const struct request *rq)
+{
+	struct hd_struct *part = rcu_dereference(rq->part);
+	return part;
+}
+
 /*
  * Request issue related functions.
  */

Quoted 9 lines · Jerome Marchand

Quoted 7 lines · Jens Axboe

No, that's not it at all. What I mean (and what was reverted) was caching the partition lookup, and using that for the stats. The problem with that approach turned out to be the elevator queiscing logic not being fully correct. One easier way to fix that would be to reference count the part stats, instead of having to drain the queue.

Quoted 3 lines

The partition is already deleted in a rcu callback function. If I understand RCU correctly, we just need to use rcu_dereference() each time we use rq->part. Something like the following *untested* patch.

Jerome,

I don't think that rcu_dereference() is going to solve the problem. The partition table will be freed as soon as one rcu period is over. So to make sure partition table is not freed one has to be holding rcu_read_lock(). It is not a good idea to keep rcu period going till request finishes so a better idea will to to reference count it.

/proc/diskstats would display a strange output as follows.

block/blk-core.c +2 -2

index 4ce953f..064921d 100644
@@ -64,13 +64,16 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 
-	if (!new_io)
+	if (!new_io) {
+		part = rq->part;
 		part_stat_inc(cpu, part, merges[rw]);

Quoted 2 lines · Jerome Marchand

/proc/diskstats would display a strange output as follows.

[snip]

This looks a lot better! One comment:

Quoted 14 lines

block/blk-core.c +2 -2

index 4ce953f..064921d 100644
@@ -64,13 +64,16 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 
-	if (!new_io)
+	if (!new_io) {
+		part = rq->part;
 		part_stat_inc(cpu, part, merges[rw]);

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 79

Quoted 10 lines

block/blk-core.c +4 -1

-	else {
+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 		part_round_stats(cpu, part);
 		part_inc_in_flight(part, rw);
+		kref_get(&part->ref);
+		rq->part = part;
 	}
 
 	part_stat_unlock();

I don't think this is completely safe. The rcu lock is held due to the part_stat_lock(), but that only prevents the __delete_partition() callback from happening. Lets say you have this:

CPU0 CPU1
part = disk_map_sector_rcu()

                                             kref_put(part); <- now 0
part_stat_unlock()
                                             __delete_partition();
                                             ...
                                             delete_partition_rcu_cb();

merge, or endio, boom

Now rq has ->part pointing to freed memory, later merges or end accounting will touch freed memory.

I think we can fix this by just having delete_partition_rcu_rb() check the reference count and return if non-zero. Since someone holds a reference to the table, they will drop it and we'll re-schedule the rcu callback.

Quoted 2 lines · Jerome Marchand

/proc/diskstats would display a strange output as follows.

[snip]

This looks a lot better! One comment:

Quoted 14 lines

block/blk-core.c +2 -2

index 4ce953f..064921d 100644
@@ -64,13 +64,16 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 
-	if (!new_io)
+	if (!new_io) {
+		part = rq->part;
 		part_stat_inc(cpu, part, merges[rw]);

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 79

Quoted 10 lines

block/blk-core.c +4 -1

-	else {
+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 		part_round_stats(cpu, part);
 		part_inc_in_flight(part, rw);
+		kref_get(&part->ref);
+		rq->part = part;
 	}
 
 	part_stat_unlock();

I don't think this is completely safe. The rcu lock is held due to the part_stat_lock(), but that only prevents the __delete_partition() callback from happening. Lets say you have this:

CPU0 CPU1
part = disk_map_sector_rcu()

                                             kref_put(part); <- now 0

part_stat_unlock()

                                             __delete_partition();
                                             ...
                                             delete_partition_rcu_cb();

merge, or endio, boom

Now rq has ->part pointing to freed memory, later merges or end accounting will touch freed memory.

I think we can fix this by just having delete_partition_rcu_rb() check the reference count and return if non-zero. Since someone holds a reference to the table, they will drop it and we'll re-schedule the rcu callback.

Quoted 37 lines · Jens Axboe

Quoted 2 lines · Jerome Marchand

/proc/diskstats would display a strange output as follows.

[snip]

This looks a lot better! One comment:

Quoted 14 lines

block/blk-core.c +2 -2

index 4ce953f..064921d 100644
@@ -64,13 +64,16 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 
-	if (!new_io)
+	if (!new_io) {
+		part = rq->part;
 		part_stat_inc(cpu, part, merges[rw]);

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 79

Quoted 10 lines

block/blk-core.c +4 -1

-	else {
+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
 		part_round_stats(cpu, part);
 		part_inc_in_flight(part, rw);
+		kref_get(&part->ref);
+		rq->part = part;
 	}
 
 	part_stat_unlock();

I don't think this is completely safe. The rcu lock is held due to the part_stat_lock(), but that only prevents the __delete_partition() callback from happening. Lets say you have this:

Quoted 3 lines

CPU0 CPU1
part = disk_map_sector_rcu()

                                             kref_put(part); <- now 0

A kref_get(part) happens here, or am I missing something?

If we use an atomic kref_test_and_get() function, which only increment the refcount if it's not zero, we would be safe. If kref_test_and_get() fails, we can cache rq->rq_disk->part0 instead. See my drafty patch below.

Quoted 7 lines

part_stat_unlock()

                                             __delete_partition();
                                             ...
                                             delete_partition_rcu_cb();

merge, or endio, boom

Quoted 9 lines

Now rq has ->part pointing to freed memory, later merges or end accounting will touch freed memory.

I think we can fix this by just having delete_partition_rcu_rb() check the reference count and return if non-zero. Since someone holds a reference to the table, they will drop it and we'll re-schedule the rcu callback.

---

block/blk-core.c +0 -1

index 4ce953f..72d12d2 100644
@@ -64,13 +64,21 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

block/blk-core.c +10 -2

+			 * The partition is already being removed,
+			 * the request will be accounted on the disk only
+			 */
+			part = &rq->rq_disk->part0;
 		part_round_stats(cpu, part);
 		part_inc_in_flight(part, rw);
+		rq->part = part;
 	}
 
 	part_stat_unlock();

@@ -128,6 +136,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->ref_count = 1;
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
+	rq->part = NULL;
 }
 EXPORT_SYMBOL(blk_rq_init);
 

@@ -1776,7 +1785,7 @@ static void blk_account_io_completion(struct request *req, unsigned int bytes)
 		int cpu;
 
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = req->part;
 		part_stat_add(cpu, part, sectors[rw], bytes >> 9);
 		part_stat_unlock();
 	}

@@ -1796,13 +1805,15 @@ static void blk_account_io_done(struct request *req)
 		int cpu;
 
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = req->part;
 
 		part_stat_inc(cpu, part, ios[rw]);
 		part_stat_add(cpu, part, ticks[rw], duration);
 		part_round_stats(cpu, part);
 		part_dec_in_flight(part, rw);
 
+		if (part->partno)
+			kref_put(&part->ref, __delete_partition);
 		part_stat_unlock();
 	}
 }

block/blk-merge.c +3 -1

index 77b7c26..3334578 100644
@@ -351,11 +351,13 @@ static void blk_account_io_merge(struct request *req)
 		int cpu;
 
 		cpu = part_stat_lock();
-		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));
+		part = req->part;
 
 		part_round_stats(cpu, part);
 		part_dec_in_flight(part, rq_data_dir(req));
 
+		if (part->partno)
+			kref_put(&part->ref, __delete_partition);
 		part_stat_unlock();
 	}
 }

block/genhd.c +1 -0

index 5fa2b44..0c55eae 100644
@@ -1192,6 +1192,7 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
 			return NULL;
 		}
 		disk->part_tbl->part[0] = &disk->part0;
+		kref_init(&disk->part0.ref);
 
 		disk->minors = minors;
 		rand_initialize_disk(disk);

fs/partitions/check.c +9 -1

index 0a8b0ad..0123717 100644
@@ -372,6 +372,13 @@ static void delete_partition_rcu_cb(struct rcu_head *head)
 	put_device(part_to_dev(part));
 }
 
+void __delete_partition(struct kref *ref)
+{
+	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
+
+	call_rcu(&part->rcu_head, delete_partition_rcu_cb);
+}
+
 void delete_partition(struct gendisk *disk, int partno)
 {
 	struct disk_part_tbl *ptbl = disk->part_tbl;

@@ -390,7 +397,7 @@ void delete_partition(struct gendisk *disk, int partno)
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
 
-	call_rcu(&part->rcu_head, delete_partition_rcu_cb);
+	kref_put(&part->ref, __delete_partition);
 }
 
 static ssize_t whole_disk_show(struct device *dev,

@@ -489,6 +496,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
 	if (!dev_get_uevent_suppress(ddev))
 		kobject_uevent(&pdev->kobj, KOBJ_ADD);
 
+	kref_init(&p->ref);
 	return p;
 
 out_free_info:

include/linux/blkdev.h +1 -0

index aae86fd..482a7fd 100644
@@ -115,6 +115,7 @@ struct request {
 	void *elevator_private3;
 
 	struct gendisk *rq_disk;
+	struct hd_struct *part;
 	unsigned long start_time;
 #ifdef CONFIG_BLK_CGROUP
 	unsigned long long start_time_ns;

include/linux/genhd.h +2 -0

index 7a7b9c1..2ba2792 100644
@@ -116,6 +116,7 @@ struct hd_struct {
 	struct disk_stats dkstats;
 #endif
 	struct rcu_head rcu_head;
+	struct kref ref;
 };
 
 #define GENHD_FL_REMOVABLE			1

@@ -583,6 +584,7 @@ extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
 						     sector_t len, int flags,
 						     struct partition_meta_info
 						       *info);
+extern void __delete_partition(struct kref *ref);
 extern void delete_partition(struct gendisk *, int);
 extern void printk_all_partitions(void);
 

include/linux/kref.h +1 -0

index 6cc38fc..90b9e44 100644
@@ -23,6 +23,7 @@ struct kref {
 
 void kref_init(struct kref *kref);
 void kref_get(struct kref *kref);
+int kref_test_and_get(struct kref *kref);
 int kref_put(struct kref *kref, void (*release) (struct kref *kref));
 
 #endif /* _KREF_H_ */

lib/kref.c +16 -0

index d3d227a..5f663b9 100644
@@ -37,6 +37,22 @@ void kref_get(struct kref *kref)
 }
 
 /**
+ * kref_test_and_get - increment refcount for object only if refcount is not
+ * zero.
+ * @kref: object.
+ *
+ * Return non-zero if the refcount was incremented, 0 otherwise
+ */
+int kref_test_and_get(struct kref *kref)
+{
+	int ret;
+	smp_mb__before_atomic_inc();
+	ret = atomic_inc_not_zero(&kref->refcount);
+	smp_mb__after_atomic_inc();
+	return ret;
+}
+
+/**
  * kref_put - decrement refcount for object.
  * @kref: object.
  * @release: pointer to the function that will clean up the object when the

Quoted 9 lines

block/blk-core.c +0 -1

index 4ce953f..72d12d2 100644
@@ -64,13 +64,21 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 74

Quoted 10 lines

block/blk-core.c +6 -2

 
-	if (!new_io)
+	if (!new_io) {
+		part = rq->part;
 		part_stat_inc(cpu, part, merges[rw]);
-	else {
+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
+		if(part->partno && !kref_test_and_get(&part->ref))
+			/*

Do we have to check this part->partno both while taking and releasing reference. Can't we take one extra reference for disk->part0, at alloc_disk_node() time so that it is never freed and only freed when disk is going away and gendisk is being freed.

That way, you don't have to differentiate between disk->part0 and rest of the partitions while taking or dropping references.

Thanks
Vivek

Quoted 25 lines · Vivek Goyal

Quoted 9 lines

block/blk-core.c +0 -1

index 4ce953f..72d12d2 100644
@@ -64,13 +64,21 @@ static void drive_stat_acct(struct request *rq, int new_io)
 		return;
 
 	cpu = part_stat_lock();
-	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 74

Quoted 10 lines

block/blk-core.c +6 -2

 
-	if (!new_io)
+	if (!new_io) {
+		part = rq->part;
 		part_stat_inc(cpu, part, merges[rw]);
-	else {
+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
+		if(part->partno && !kref_test_and_get(&part->ref))
+			/*

Do we have to check this part->partno both while taking and releasing reference. Can't we take one extra reference for disk->part0, at alloc_disk_node() time so that it is never freed and only freed when disk is going away and gendisk is being freed.

Quoted 2 lines

That way, you don't have to differentiate between disk->part0 and rest of the partitions while taking or dropping references.

We could do it in any way, as long as we don't end up trying to free disk->part0. I choose not to touch part0->ref at all, but we could also drop all the part->partno test, and get a reference on part0 when we use it as a backup. I have no strong opinion about a way or an other.

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object.

If the object is "being removed", then it will return
NULL and you need to check that. Do that and you do not need to add:

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 73

Quoted 2 lines

block/blk-core.c +1 -0

+		if (!kref_test_and_get(&part->ref)) {

At all.

thanks,

greg k-h

Quoted 5 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

Quoted 8 lines

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object.

Agreed.

Quoted 2 lines

If the object is "being removed", then it will return
NULL and you need to check that. Do that and you do not need to add:

The object is actually removed in a rcu callback function. We could certainly add a flag to hd_struct, set by the release function, to indicate disk_map_sector_rcu() that the partition is being removed, but why not use the refcount instead?

Quoted 17 lines · Jerome Marchand

Quoted 5 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

Quoted 8 lines

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object.

Agreed.

Quoted 7 lines

Quoted 2 lines

If the object is "being removed", then it will return
NULL and you need to check that. Do that and you do not need to add:

The object is actually removed in a rcu callback function. We could certainly add a flag to hd_struct, set by the release function, to indicate disk_map_sector_rcu() that the partition is being removed, but why not use the refcount instead?

Because you have to properly serialize the grabbing of a kref if you don't have a valid pointer in the first place, otherwise it will not work properly at all. Your new function still does not properly handle the race condition of dropping the last reference and then having the kref be cleaned up. You are giving false hope to the user of the api that what they are doing is correct.

Quoted 26 lines · Greg KH

Quoted 17 lines · Jerome Marchand

Quoted 5 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

Quoted 8 lines

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object.

Agreed.

Quoted 7 lines

Quoted 2 lines

If the object is "being removed", then it will return
NULL and you need to check that. Do that and you do not need to add:

The object is actually removed in a rcu callback function. We could certainly add a flag to hd_struct, set by the release function, to indicate disk_map_sector_rcu() that the partition is being removed, but why not use the refcount instead?

Quoted 7 lines

Because you have to properly serialize the grabbing of a kref if you don't have a valid pointer in the first place, otherwise it will not work properly at all. Your new function still does not properly handle the race condition of dropping the last reference and then having the kref be cleaned up. You are giving false hope to the user of the api that what they are doing is correct.

For clarification, is your objection only about not adding that misleading function to kref api (I understand that), or is my code actually racy?

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

Quoted 8 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

Quoted 6 lines

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

It doesn't matter if you do it in there of after the fact, since the "lock" (RCU) is being held across the call. See my original suggestion here:

https://lkml.org/lkml/2010/12/17/275

Quoted 16 lines · Jens Axboe

Quoted 14 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

Quoted 5 lines

It doesn't matter if you do it in there of after the fact, since the "lock" (RCU) is being held across the call. See my original suggestion here:

https://lkml.org/lkml/2010/12/17/275

Ok, that's fine, just do it without adding that kref function and I have no objection :)

Quoted 17 lines · Greg KH

Quoted 16 lines · Jens Axboe

Quoted 14 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

Quoted 8 lines

Quoted 5 lines

It doesn't matter if you do it in there of after the fact, since the "lock" (RCU) is being held across the call. See my original suggestion here:

https://lkml.org/lkml/2010/12/17/275

Ok, that's fine, just do it without adding that kref function and I have no objection :)

Why? The code is perfectly fine. I originally objected to making an API like this for simple reference counting - seems I was right. Please actually look at the code and use. Alexey asked whether this was a toy API or a real one, I'd like to know that as well. If this is meant just for very basic get/put references, fine, then document that. But then what's the point of having this API in the first place?

Quoted 27 lines · Jens Axboe

Quoted 17 lines · Greg KH

Quoted 16 lines · Jens Axboe

Quoted 14 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 72

Quoted 2 lines

block/blk-core.c +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

Quoted 8 lines

Quoted 5 lines

It doesn't matter if you do it in there of after the fact, since the "lock" (RCU) is being held across the call. See my original suggestion here:

https://lkml.org/lkml/2010/12/17/275

Ok, that's fine, just do it without adding that kref function and I have no objection :)

Quoted 6 lines

Why? The code is perfectly fine. I originally objected to making an API like this for simple reference counting - seems I was right. Please actually look at the code and use. Alexey asked whether this was a toy API or a real one, I'd like to know that as well. If this is meant just for very basic get/put references, fine, then document that. But then what's the point of having this API in the first place?

The point is that you shouldn't have to roll your own reference count code all over the place, 99% of the time, you should just use the debugged, and documented, interface that the kernel provides with the kref interface.

As for it being a "toy", it properly handles a very large majority of the kernel reference counting logic today, in a race-free manner, so I would not call that a "toy" at all.

Just use it properly. As this patch series points out, adding this type

of function to the api is not a good idea, as it will be incorrect when used.

Quoted 40 lines · Greg KH

Quoted 27 lines · Jens Axboe

Quoted 17 lines · Greg KH

Quoted 16 lines · Jens Axboe

Quoted 14 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

Quoted 8 lines

Quoted 5 lines

It doesn't matter if you do it in there of after the fact, since the "lock" (RCU) is being held across the call. See my original suggestion here:

https://lkml.org/lkml/2010/12/17/275

Ok, that's fine, just do it without adding that kref function and I have no objection :)

Quoted 6 lines

Why? The code is perfectly fine. I originally objected to making an API like this for simple reference counting - seems I was right. Please actually look at the code and use. Alexey asked whether this was a toy API or a real one, I'd like to know that as well. If this is meant just for very basic get/put references, fine, then document that. But then what's the point of having this API in the first place?

The point is that you shouldn't have to roll your own reference count code all over the place, 99% of the time, you should just use the debugged, and documented, interface that the kernel provides with the kref interface.

Quoted 3 lines

As for it being a "toy", it properly handles a very large majority of the kernel reference counting logic today, in a race-free manner, so I would not call that a "toy" at all.

Dunno, then perhaps pointless. It's not like the API is saving lots of typing or easier to use than just atomics, imho.

Quoted 1 line

Just use it properly. As this patch series points out, adding this type

By adding pointless locks?

Your suggestion of doing the referencing
inside the function being called is moot, since RCU is held off over the call. The point of the addition to the API is to _not_ grab a reference if someone has done the final put. We know that the RCU grace period has not ended, so the kref is valid. But if it is going away _in the future_ after we drop the part lock, then we don't want a reference to it.

So to "use it properly", I would have to slow down a fast path. No thanks.

Quoted 2 lines

of function to the api is not a good idea, as it will be incorrect when used.

The code is fine, the use is fine. I think the only thing we have established here is that Jerome made the mistake of using the kref API for this. I'll rewrite that part to handle its own references.

Quoted 41 lines · Jens Axboe

Quoted 40 lines · Greg KH

Quoted 27 lines · Jens Axboe

Quoted 17 lines · Greg KH

Quoted 16 lines · Jens Axboe

Quoted 14 lines · Greg KH

Quoted 3 lines · Jerome Marchand

Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away.

No, don't do this, use a kref correctly and no such function should be needed.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));

That is the function that should properly increment the reference count on the object. If the object is "being removed", then it will return NULL and you need to check that. Do that and you do not need to add:

Quoted 8 lines

Quoted 5 lines

It doesn't matter if you do it in there of after the fact, since the "lock" (RCU) is being held across the call. See my original suggestion here:

https://lkml.org/lkml/2010/12/17/275

Ok, that's fine, just do it without adding that kref function and I have no objection :)

Quoted 6 lines

Why? The code is perfectly fine. I originally objected to making an API like this for simple reference counting - seems I was right. Please actually look at the code and use. Alexey asked whether this was a toy API or a real one, I'd like to know that as well. If this is meant just for very basic get/put references, fine, then document that. But then what's the point of having this API in the first place?

The point is that you shouldn't have to roll your own reference count code all over the place, 99% of the time, you should just use the debugged, and documented, interface that the kernel provides with the kref interface.

Quoted 6 lines

Quoted 3 lines

As for it being a "toy", it properly handles a very large majority of the kernel reference counting logic today, in a race-free manner, so I would not call that a "toy" at all.

Dunno, then perhaps pointless. It's not like the API is saving lots of typing or easier to use than just atomics, imho.

Remember what Andrew said when you complained about this last time? I'll paraphrase:
When we see someone use an atomic value for a reference count, we then need to go audit the code to make sure they got the flushing right, and all the other little stuff you need to do in order to ensure the code works properly.

If you use a kref, then we know that all is correct, and we can then focus on how the kref is used (very easy to audit) and worry about the rest of the code. It saves us time as reviewers and maintainers also.

Quoted 3 lines

Quoted 1 line

Just use it properly. As this patch series points out, adding this type

By adding pointless locks?

No.

Quoted 7 lines

Your suggestion of doing the referencing
inside the function being called is moot, since RCU is held off over the call. The point of the addition to the API is to _not_ grab a reference if someone has done the final put. We know that the RCU grace period has not ended, so the kref is valid. But if it is going away _in the future_ after we drop the part lock, then we don't want a reference to it.

Quoted 2 lines

So to "use it properly", I would have to slow down a fast path. No thanks.

Then don't use it. a kref is NOT for a fast path, use RCU for that. Heck, an atomic value is also not good for a fast path also, as it causes major stalls, so you might want to reconsider not even using that if you are thinking you should roll your own.

Quoted 6 lines

Quoted 2 lines

of function to the api is not a good idea, as it will be incorrect when used.

The code is fine, the use is fine. I think the only thing we have established here is that Jerome made the mistake of using the kref API for this. I'll rewrite that part to handle its own references.

Yes, that is true. If you are already using RCU, then by all means, don't use a kref. There are lots of reference counts in the kernel that don't use a kref, nor should they.

block/blk-core.c +4 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
+		if (!kref_test_and_get(&part->ref)) {
+			/*

Quoted 11 lines · Jerome Marchand

block/blk-core.c +11 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
+		if (!kref_test_and_get(&part->ref)) {
+			/*
+			 * The partition is already being removed,
+			 * the request will be accounted on the disk only
+			 *
+			 * We take a reference on disk->part0 although that
+			 * partition will never be deleted, so we can treat
+			 * it as any other partition.
+			 */

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 84

Quoted 3 lines

block/blk-core.c +3 -0

+			part = &rq->rq_disk->part0;
+			kref_get(&part->ref);
+		}

This still doesn't work. So you are inside the if {} block, you know what someone has dropped the last reference and the call_rcu() is being scheduled. Adding a reference now will not prevent 'part' from going away as soon as you do part_stat_unlock().

As per my last email, you
need a check in the rcu callback to ensure that the ref is 0.

Quoted 12 lines · Jens Axboe

Quoted 11 lines · Jerome Marchand

block/blk-core.c +11 -0

+	} else {
+		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
+		if (!kref_test_and_get(&part->ref)) {
+			/*
+			 * The partition is already being removed,
+			 * the request will be accounted on the disk only
+			 *
+			 * We take a reference on disk->part0 although that
+			 * partition will never be deleted, so we can treat
+			 * it as any other partition.
+			 */

Quoted 8 lines

block/blk-core.c static void drive_stat_acct(struct request *rq, int new_io) · line 84

Quoted 3 lines

block/blk-core.c +3 -0

+			part = &rq->rq_disk->part0;
+			kref_get(&part->ref);
+		}

This still doesn't work. So you are inside the if {} block, you know what someone has dropped the last reference and the call_rcu() is being scheduled. Adding a reference now will not prevent 'part' from going away as soon as you do part_stat_unlock().

And what is the problem with that since we don't use 'part' (as returned by disk_map_sector_rcu()), but disk->part0 instead?

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

Signed-off-by Jerome Marchand <[email protected]>

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

Quoted 3 lines · Jerome Marchand

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

Is kref supposed to be used only for "toy" structures where you aren't expected to do get possibly dead object?

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

Quoted 4 lines · Jerome Marchand

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

We just removed a function like this recently as it really isn't the solution for what you need here at all.

Please use a lock that protects the desctruction of the kref, like the rest of the kernel, to keep someone from grabing a reference while the device could be going away. That's the only way to safely do this.

See the archives for why this overall, isn't a good idea for a kref to have this type of function.

thanks,

greg k-h

Quoted 6 lines · Greg KH

Quoted 4 lines · Jerome Marchand

Add kref_test_and_get() function, which atomically add a reference only if refcount is not zero. This prevent to add a reference to an object that is already being removed.

Quoted 2 lines

We just removed a function like this recently as it really isn't the solution for what you need here at all.

What function are you talking about? kref_set? If yes, I don't see the similarity between this function and mine.