dm.c - device-mapper I/O path fixes

PATCH In review Kevin Corry 6 participants 42 messages Dec 10, 2002 – Dec 16, 2002

Subsystems (3)

Memory management MM core Device mapper

lore

Share & tools

lore

Kevin Corry

author

2002-12-10 22:42 UTC #0 lore raw

PATCH 1/1 dm.c - device-mapper I/O path fixes

Joe, Linus,

This patch fixes problems with the device-mapper I/O path in 2.5.51. The existing code does not properly split requests when necessary, and can cause segfaults and/or data corruption. This can easily manifest itself when running XFS on striped LVM volumes.

Notes:
- New bio's must be alloc'd instead of clone'd, since the bio vector may need to be adjusted if the end of the split request does not fill a page.
- Fix reference counting of md->pending. This should only be incremented once for each incoming bio, not for each split bio that is resubmitted.
- Copy the correct bvec when splitting the tail-end of a page.

Signature · 177 lines

Kevin Corry
[email protected]
http://evms.sourceforge.net/


--- linux-2.5.51a/drivers/md/dm.c	Tue Dec 10 11:01:13 2002
+++ linux-2.5.51b/drivers/md/dm.c	Tue Dec 10 11:03:55 2002
@@ -242,17 +242,18 @@
 	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
 	unsigned long flags;
 
-	spin_lock_irqsave(&_uptodate_lock, flags);
-	if (error)
+	if (error) {
+		spin_lock_irqsave(&_uptodate_lock, flags);
 		io->error = error;
-	spin_unlock_irqrestore(&_uptodate_lock, flags);
+		spin_unlock_irqrestore(&_uptodate_lock, flags);
+	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))
 			/* nudge anyone waiting on suspend queue */
 			wake_up(&io->md->wait);
 
-		bio_endio(io->bio, io->error ? 0 : io->bio->bi_size, io->error);
+		bio_endio(io->bio, io->bio->bi_size, io->error);
 		free_io(io);
 	}
 }
@@ -261,15 +262,15 @@
 {
 	struct dm_io *io = bio->bi_private;
 
-	/*
-	 * Only call dec_pending if the clone has completely
-	 * finished.  If a partial io errors I'm assuming it won't
-	 * be requeued.  FIXME: check this.
-	 */
-	if (error || !bio->bi_size) {
-		dec_pending(io, error);
-		bio_put(bio);
+	if (bio->bi_size)
+		return 1;
+
+	if (error) {
+		struct gendisk *disk = dm_disk(io->md);
+		DMWARN("I/O error (%d) on device %s\n", error, disk->disk_name);
 	}
+	dec_pending(io, error);
+	bio_put(bio);
 
 	return 0;
 }
@@ -313,7 +314,6 @@
 	 * anything, the target has assumed ownership of
 	 * this io.
 	 */
-	atomic_inc(&io->md->pending);
 	atomic_inc(&io->io_count);
 	r = ti->type->map(ti, clone);
 	if (r > 0)
@@ -341,9 +341,7 @@
 {
 	struct dm_target *ti = dm_table_find_target(ci->md->map, ci->sector);
 	struct bio *clone, *bio = ci->bio;
-	struct bio_vec *bv = bio->bi_io_vec + (bio->bi_vcnt - 1);
-
-	DMWARN("splitting page");
+	struct bio_vec *bv = bio->bi_io_vec + ci->idx;
 
 	if (len > ci->sector_count)
 		len = ci->sector_count;
@@ -353,11 +351,13 @@
 
 	clone->bi_sector = ci->sector;
 	clone->bi_bdev = bio->bi_bdev;
-	clone->bi_flags = bio->bi_flags | (1 << BIO_SEG_VALID);
 	clone->bi_rw = bio->bi_rw;
+	clone->bi_vcnt = 1;
 	clone->bi_size = len << SECTOR_SHIFT;
 	clone->bi_end_io = clone_endio;
 	clone->bi_private = ci->io;
+	clone->bi_io_vec->bv_offset = clone->bi_io_vec->bv_len - clone->bi_size;
+	clone->bi_io_vec->bv_len = clone->bi_size;
 
 	ci->sector += len;
 	ci->sector_count -= len;
@@ -369,24 +369,48 @@
 {
 	struct bio *clone, *bio = ci->bio;
 	struct dm_target *ti = dm_table_find_target(ci->md->map, ci->sector);
-	sector_t len = max_io_len(ci->md, bio->bi_sector, ti);
+	sector_t bv_len, len = max_io_len(ci->md, ci->sector, ti);
+	struct bio_vec *bv;
+	int i, vcnt = bio->bi_vcnt - ci->idx;
 
 	/* shorter than current target ? */
 	if (ci->sector_count < len)
 		len = ci->sector_count;
 
 	/* create the clone */
-	clone = bio_clone(ci->bio, GFP_NOIO);
+	clone = bio_alloc(GFP_NOIO, vcnt);
+	if (!clone) {
+		dec_pending(ci->io, -ENOMEM);
+		return;
+	}
 	clone->bi_sector = ci->sector;
-	clone->bi_idx = ci->idx;
+	clone->bi_bdev = bio->bi_bdev;
+	clone->bi_rw = bio->bi_rw;
+	clone->bi_vcnt = vcnt;
 	clone->bi_size = len << SECTOR_SHIFT;
 	clone->bi_end_io = clone_endio;
 	clone->bi_private = ci->io;
 
+	/* copy the original vector and adjust if necessary. */
+	memcpy(clone->bi_io_vec, bio->bi_io_vec + ci->idx,
+	       vcnt * sizeof(*clone->bi_io_vec));
+	bv_len = len << SECTOR_SHIFT;
+	bio_for_each_segment(bv, clone, i) {
+		if (bv_len >= bv->bv_len) {
+			bv_len -= bv->bv_len;
+		} else {
+			bv->bv_len = bv_len;
+			clone->bi_vcnt = i + 1;
+			break;
+		}
+	}
+
+	/* submit this io */
+	__map_bio(ti, clone);
+
 	/* adjust the remaining io */
 	ci->sector += len;
 	ci->sector_count -= len;
-	__map_bio(ti, clone);
 
 	/*
 	 * If we are not performing all remaining io in this
@@ -395,8 +419,8 @@
 	 */
 	if (ci->sector_count) {
 		while (len) {
-			struct bio_vec *bv = clone->bi_io_vec + ci->idx;
-			sector_t bv_len = bv->bv_len >> SECTOR_SHIFT;
+			bv = bio->bi_io_vec + ci->idx;
+			bv_len = bv->bv_len >> SECTOR_SHIFT;
 			if (bv_len <= len)
 				len -= bv_len;
 
@@ -427,6 +451,8 @@
 	ci.sector_count = bio_sectors(bio);
 	ci.idx = 0;
 
+	atomic_inc(&ci.io->md->pending);
+
 	while (ci.sector_count)
 		__clone_and_map(&ci);
 
@@ -457,13 +483,13 @@
 		up_read(&md->lock);
 
 		if (bio_rw(bio) == READA) {
-			bio_io_error(bio, 0);
+			bio_io_error(bio, bio->bi_size);
 			return 0;
 		}
 
 		r = queue_io(md, bio);
 		if (r < 0) {
-			bio_io_error(bio, 0);
+			bio_io_error(bio, bio->bi_size);
 			return 0;
 
 		} else if (r == 0)

Joe Thornber

2002-12-11 12:10 UTC #1 lore raw

Kevin,

Quoted 6 lines · Kevin Corry

Joe, Linus,
This patch fixes problems with the device-mapper I/O path in 2.5.51. The existing code does not properly split requests when necessary, and can cause segfaults and/or data corruption. This can easily manifest itself when running XFS on striped LVM volumes.

Many thanks for doing this work, but _please_ split your patches up more. There are several changes rolled in here.

I've split the patch up, and will post the ones I'm accepting as replies to this current mail.

The full set of changes for 2.5.51 is available here:
http://people.sistina.com/~thornber/patches/2.5-stable/2.5.51/

This works for me with xfs and stripes (limited testing).

These are the bits of your patch that I have queries about:

drivers/md/dm.c +1 -1

@@ -242,4 +242,4 @@
-		bio_endio(io->bio, io->error ? 0 : io->bio->bi_size, io->error);
+		bio_endio(io->bio, io->bio->bi_size, io->error);

You seem to be assuming that io->bio->bi_size will always be zero if an error occurs. I was not aware that this was the case.

@@ -261,15 +262,15 @@
{
struct dm_io *io = bio->bi_private;

- /*
- * Only call dec_pending if the clone has completely
- * finished. If a partial io errors I'm assuming it won't
- * be requeued. FIXME: check this.
- */
- if (error || !bio->bi_size) {
- dec_pending(io, error);
- bio_put(bio);
+ if (bio->bi_size)
+ return 1;
+
+ if (error) {
+ struct gendisk *disk = dm_disk(io->md);
+ DMWARN("I/O error (%d) on device %s\n", error, disk->disk_name);
}
+ dec_pending(io, error);
+ bio_put(bio);

return 0;
}

All you're doing here is adding a warning (which is nice), and making the same assumption about bio->bi_size in the case of an error.

@@ -457,13 +483,13 @@
up_read(&md->lock);

if (bio_rw(bio) == READA) {
- bio_io_error(bio, 0);
+ bio_io_error(bio, bio->bi_size);
return 0;
}

r = queue_io(md, bio);
if (r < 0) {
- bio_io_error(bio, 0);
+ bio_io_error(bio, bio->bi_size);
return 0;

} else if (r == 0)

Why is it better to say that all the io was 'done' rather than none? It did fail after all.

@@ -369,24 +369,48 @@
{
struct bio *clone, *bio = ci->bio;
struct dm_target *ti = dm_table_find_target(ci->md->map, ci->sector);
- sector_t len = max_io_len(ci->md, bio->bi_sector, ti);
+ sector_t bv_len, len = max_io_len(ci->md, ci->sector, ti);
+ struct bio_vec *bv;
+ int i, vcnt = bio->bi_vcnt - ci->idx;

/* shorter than current target ? */
if (ci->sector_count < len)
len = ci->sector_count;

/* create the clone */
- clone = bio_clone(ci->bio, GFP_NOIO);
+ clone = bio_alloc(GFP_NOIO, vcnt);
+ if (!clone) {
+ dec_pending(ci->io, -ENOMEM);
+ return;
+ }
clone->bi_sector = ci->sector;
- clone->bi_idx = ci->idx;
+ clone->bi_bdev = bio->bi_bdev;
+ clone->bi_rw = bio->bi_rw;
+ clone->bi_vcnt = vcnt;
clone->bi_size = len << SECTOR_SHIFT;
clone->bi_end_io = clone_endio;
clone->bi_private = ci->io;

+ /* copy the original vector and adjust if necessary. */
+ memcpy(clone->bi_io_vec, bio->bi_io_vec + ci->idx,
+ vcnt * sizeof(*clone->bi_io_vec));
+ bv_len = len << SECTOR_SHIFT;
+ bio_for_each_segment(bv, clone, i) {
+ if (bv_len >= bv->bv_len) {
+ bv_len -= bv->bv_len;
+ } else {
+ bv->bv_len = bv_len;
+ clone->bi_vcnt = i + 1;
+ break;
+ }
+ }
+
+ /* submit this io */
+ __map_bio(ti, clone);
+
/* adjust the remaining io */
ci->sector += len;
ci->sector_count -= len;
- __map_bio(ti, clone);

/*
* If we are not performing all remaining io in this
@@ -395,9 +419,9 @@
*/
if (ci->sector_count) {
while (len) {
- struct bio_vec *bv = clone->bi_io_vec + ci->idx;
- sector_t bv_len = bv->bv_len >> SECTOR_SHIFT;
+ bv = bio->bi_io_vec + ci->idx;
+ bv_len = bv->bv_len >> SECTOR_SHIFT;
if (bv_len <= len)
len -= bv_len;

There is no need to use bio_alloc in preference to bio_clone, we're not changing the bvec in any way. All of the above code is redundant.

- Joe

Joe Thornber

2002-12-11 12:11 UTC #2 lore raw

dec_pending(): only bother spin locking if io->error is going to be updated. [Kevin Corry]

drivers/md/dm.c +4 -3

@@ -238,10 +238,11 @@
 	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
 	unsigned long flags;
 
-	spin_lock_irqsave(&_uptodate_lock, flags);
-	if (error)
+	if (error) {
+		spin_lock_irqsave(&_uptodate_lock, flags);
 		io->error = error;
-	spin_unlock_irqrestore(&_uptodate_lock, flags);
+		spin_unlock_irqrestore(&_uptodate_lock, flags);
+	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))

Denis Vlasenko

2002-12-11 13:35 UTC #3 lore raw

Quoted 10 lines · Joe Thornber

dec_pending(): only bother spin locking if io->error is going to be updated. [Kevin Corry]
drivers/md/dm.c +0 -1
@@ -238,10 +238,11 @@
 	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
 	unsigned long flags;
 
-	spin_lock_irqsave(&_uptodate_lock, flags);

drivers/md/dm.c line 248

Quoted 10 lines

drivers/md/dm.c +4 -2

-	if (error)
+	if (error) {
+		spin_lock_irqsave(&_uptodate_lock, flags);
 		io->error = error;
-	spin_unlock_irqrestore(&_uptodate_lock, flags);
+		spin_unlock_irqrestore(&_uptodate_lock, flags);
+	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))

This seems pointless, end result:

	spin_lock_irqsave(&_uptodate_lock, flags);
 	io->error = error;
	spin_unlock_irqrestore(&_uptodate_lock, flags);

BTW, the function we are looking at:

static inline void dec_pending(struct dm_io *io, int error)

       ^^^^^^

is too big for inlining.
--
vda

Kevin Corry

author

2002-12-11 13:55 UTC #4 lore raw

Quoted 19 lines · Denis Vlasenko

Quoted 18 lines · Joe Thornber

dec_pending(): only bother spin locking if io->error is going to be updated. [Kevin Corry]

drivers/md/dm.c +4 -3

@@ -238,10 +238,11 @@
 	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
 	unsigned long flags;
 
-	spin_lock_irqsave(&_uptodate_lock, flags);
-	if (error)
+	if (error) {
+		spin_lock_irqsave(&_uptodate_lock, flags);
 		io->error = error;
-	spin_unlock_irqrestore(&_uptodate_lock, flags);
+		spin_unlock_irqrestore(&_uptodate_lock, flags);
+	}

Quoted 8 lines

drivers/md/dm.c line 248

Quoted 2 lines

drivers/md/dm.c +0 -0

 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))

This seems pointless, end result:

	spin_lock_irqsave(&_uptodate_lock, flags);
 	io->error = error;
	spin_unlock_irqrestore(&_uptodate_lock, flags);

Are you saying the "if (error)" part is pointless? If so, I have to disagree. A bio may be split into several sub-bio's. When each of those split bio's completes, they are going to call this function. But if only one of those split bio's has an error, then the error might get lost without that "if" statement.

However, it might be a good idea to consider how bio's keep track of errors. When a bio is created, it is marked UPTODATE. Then, if any part of a bio takes an error, the UPTODATE flag is turned off. When the whole bio completes, if the UPTODATE flag is still on, there were no errors during the i/o. Perhaps the "error" field in "struct dm_io" could be modified to use this method of error tracking? Then we could change dec_pending() to be something like:

if (error)

	clear_bit(DM_IO_UPTODATE, &io->error);

with a "set_bit(DM_IO_UPTODATE, &ci.io->error);" in __bio_split().

Signature · 3 lines

Kevin Corry
[email protected]
http://evms.sourceforge.net/

Joe Thornber

2002-12-11 14:10 UTC #5 lore raw

Quoted 8 lines · Kevin Corry

However, it might be a good idea to consider how bio's keep track of errors. When a bio is created, it is marked UPTODATE. Then, if any part of a bio takes an error, the UPTODATE flag is turned off. When the whole bio completes, if the UPTODATE flag is still on, there were no errors during the i/o. Perhaps the "error" field in "struct dm_io" could be modified to use this method of error tracking? Then we could change dec_pending() to be something like:

Quoted 4 lines

if (error)
	clear_bit(DM_IO_UPTODATE, &io->error);
with a "set_bit(DM_IO_UPTODATE, &ci.io->error);" in __bio_split().

The problem with this is you don't keep track of the specific error to later pass to bio_endio(io->bio...). I guess it all comes down to just how expensive that spin lock is; and since locking only occurs when there's an error I'm happy with things as they are.

- Joe

Denis Vlasenko

2002-12-11 14:36 UTC #6 lore raw

Quoted 12 lines · Joe Thornber

Quoted 11 lines · Kevin Corry
However, it might be a good idea to consider how bio's keep track of errors. When a bio is created, it is marked UPTODATE. Then, if any part of a bio takes an error, the UPTODATE flag is turned off. When the whole bio completes, if the UPTODATE flag is still on, there were no errors during the i/o. Perhaps the "error" field in "struct dm_io" could be modified to use this method of error tracking? Then we could change dec_pending() to be something like:
if (error)
	clear_bit(DM_IO_UPTODATE, &io->error);

Quoted 6 lines

Quoted 1 line
with a "set_bit(DM_IO_UPTODATE, &ci.io->error);" in __bio_split().
The problem with this is you don't keep track of the specific error to later pass to bio_endio(io->bio...). I guess it all comes down to just how expensive that spin lock is; and since locking only occurs when there's an error I'm happy with things as they are.

lock();
a = b;
unlock();

Store of ints is atomic anyway. You need locking if a is a larger entity, say, a struct.
--
vda

Kevin Corry

author

2002-12-11 14:44 UTC #7 lore raw

Quoted 20 lines · Denis Vlasenko

Quoted 12 lines · Joe Thornber
Quoted 11 lines · Kevin Corry
However, it might be a good idea to consider how bio's keep track of errors. When a bio is created, it is marked UPTODATE. Then, if any part of a bio takes an error, the UPTODATE flag is turned off. When the whole bio completes, if the UPTODATE flag is still on, there were no errors during the i/o. Perhaps the "error" field in "struct dm_io" could be modified to use this method of error tracking? Then we could change dec_pending() to be something like:
if (error)
	clear_bit(DM_IO_UPTODATE, &io->error);
Quoted 6 lines
Quoted 1 line
with a "set_bit(DM_IO_UPTODATE, &ci.io->error);" in __bio_split().
The problem with this is you don't keep track of the specific error to later pass to bio_endio(io->bio...). I guess it all comes down to just how expensive that spin lock is; and since locking only occurs when there's an error I'm happy with things as they are.

Quoted 6 lines

lock();
a = b;
unlock();
Store of ints is atomic anyway. You need locking if a is a larger entity, say, a struct.

Storing an int is *not* atomic unless it is declared as atomic_t and you use the appropriate macros (see include/asm-*/atomic.h). Remember, we are talking about a field in a data structure that can be accessed from multiple threads on multiple CPUs.

Signature · 3 lines

Kevin Corry
[email protected]
http://evms.sourceforge.net/

Paul Mackerras

2002-12-11 21:04 UTC #8 lore raw

Quoted 4 lines · Kevin Corry

Storing an int is *not* atomic unless it is declared as atomic_t and you use the appropriate macros (see include/asm-*/atomic.h). Remember, we are talking about a field in a data structure that can be accessed from multiple threads on multiple CPUs.

As a practical matter, I believe that storing an int to an int-aligned address _is_ actually atomic on any CPU that can run Linux. The PowerPC architecture spec requires that single-word (i.e. 32-bit) aligned stores are atomic, for instance, and I think that would be the case on any other sane architecture as well.

The language lawyers would probably agree with you, though.

Regards,
Paul.

Kevin Corry

author

2002-12-12 13:10 UTC #9 lore raw

Quoted 11 lines · Paul Mackerras

Quoted 4 lines · Kevin Corry
Storing an int is *not* atomic unless it is declared as atomic_t and you use the appropriate macros (see include/asm-*/atomic.h). Remember, we are talking about a field in a data structure that can be accessed from multiple threads on multiple CPUs.
As a practical matter, I believe that storing an int to an int-aligned address _is_ actually atomic on any CPU that can run Linux. The PowerPC architecture spec requires that single-word (i.e. 32-bit) aligned stores are atomic, for instance, and I think that would be the case on any other sane architecture as well.

Given the constraints of having properly aligned data on an SMP machine with the correct cache-coherency hardware, then yes, I will agree that such stores should be atomic. However, it has been my understanding that these conditions cannot be guaranteed on every architecture. Thus we're stuck with atomic_t's so everyone can play nicely together.

Signature · 3 lines

Kevin Corry
[email protected]
http://evms.sourceforge.net/

Denis Vlasenko

2002-12-11 14:31 UTC #10 lore raw

In reply to Kevin Corry

Quoted 20 lines · Kevin Corry

Quoted 20 lines

Quoted 7 lines

drivers/md/dm.c +0 -1

@@ -238,10 +238,11 @@
 	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
 	unsigned long flags;
 
-	spin_lock_irqsave(&_uptodate_lock, flags);

drivers/md/dm.c line 248

Quoted 10 lines

drivers/md/dm.c +4 -2

-	if (error)
+	if (error) {
+		spin_lock_irqsave(&_uptodate_lock, flags);
 		io->error = error;
-	spin_unlock_irqrestore(&_uptodate_lock, flags);
+		spin_unlock_irqrestore(&_uptodate_lock, flags);
+	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))

This seems pointless, end result:

Quoted 5 lines

Quoted 3 lines
	spin_lock_irqsave(&_uptodate_lock, flags);
 	io->error = error;
	spin_unlock_irqrestore(&_uptodate_lock, flags);
Are you saying the "if (error)" part is pointless? If so, I have to

No. Locking is pointless. What exactly you try to protect here?

Quoted 13 lines

disagree. A bio may be split into several sub-bio's. When each of those split bio's completes, they are going to call this function. But if only one of those split bio's has an error, then the error might get lost without that "if" statement.
However, it might be a good idea to consider how bio's keep track of errors. When a bio is created, it is marked UPTODATE. Then, if any part of a bio takes an error, the UPTODATE flag is turned off. When the whole bio completes, if the UPTODATE flag is still on, there were no errors during the i/o. Perhaps the "error" field in "struct dm_io" could be modified to use this method of error tracking? Then we could change dec_pending() to be something like:

Quoted 4 lines

if (error)
	clear_bit(DM_IO_UPTODATE, &io->error);
with a "set_bit(DM_IO_UPTODATE, &ci.io->error);" in __bio_split().

You seem to overestimate my knowledge of this part of the kernel. :( --
vda

Kevin Corry

author

2002-12-11 14:41 UTC #11 lore raw

Quoted 21 lines · Denis Vlasenko

Quoted 20 lines · Kevin Corry

Quoted 20 lines

Quoted 7 lines

drivers/md/dm.c +0 -1

@@ -238,10 +238,11 @@
 	static spinlock_t _uptodate_lock = SPIN_LOCK_UNLOCKED;
 	unsigned long flags;
 
-	spin_lock_irqsave(&_uptodate_lock, flags);

drivers/md/dm.c line 248

Quoted 10 lines

drivers/md/dm.c +4 -2

-	if (error)
+	if (error) {
+		spin_lock_irqsave(&_uptodate_lock, flags);
 		io->error = error;
-	spin_unlock_irqrestore(&_uptodate_lock, flags);
+		spin_unlock_irqrestore(&_uptodate_lock, flags);
+	}
 
 	if (atomic_dec_and_test(&io->io_count)) {
 		if (atomic_dec_and_test(&io->md->pending))

This seems pointless, end result:

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
	spin_lock_irqsave(&_uptodate_lock, flags);
 	io->error = error;
	spin_unlock_irqrestore(&_uptodate_lock, flags);
Are you saying the "if (error)" part is pointless? If so, I have to
No. Locking is pointless. What exactly you try to protect here?

The "struct dm_io *io" that is passed to dec_pending() can be accessed by multiple threads at the same time, thus some form of locking is required.

I had been thinking about whether the "error" field could be an atomic_t, which would remove the requirement for the spinlock in dec_pending(). However, I don't know how atomic_t's behave with negative values. I know atomic_t's are only guaranteed to have 24-bits of precision, yet all arch's define atomic_t with a signed integer. Can anyone enlighten me on this?

Perhaps we could make "error" and atomic_t, and store the absolute-value of the error code, and always return -error in the bio_endio() call. Or is that just too ugly?

Signature · 3 lines

Kevin Corry
[email protected]
http://evms.sourceforge.net/

Joe Thornber

2002-12-11 15:04 UTC #12 lore raw

Quoted 3 lines · Kevin Corry

Perhaps we could make "error" and atomic_t, and store the absolute-value of the error code, and always return -error in the bio_endio() call. Or is that just too ugly?

Too ugly :)

Joe Thornber

2002-12-11 14:50 UTC #13 lore raw

In reply to Denis Vlasenko

Quoted 3 lines · Denis Vlasenko

Quoted 1 line
Are you saying the "if (error)" part is pointless? If so, I have to
No. Locking is pointless. What exactly you try to protect here?

io->error from being updated concurrently.

Joe Thornber

2002-12-11 12:12 UTC #14 lore raw