Re: [PATCH] dm.c - device-mapper I/O path fixes

[Kevin Corry]

On Wednesday 11 December 2002 12:19, Denis Vlasenko wrote:
> On 11 December 2002 10:19, Joe Thornber wrote:
> > dec_pending(): only bother spin locking if io->error is going to be
> > updated. [Kevin Corry]
> >
> > --- diff/drivers/md/dm.c	2002-12-11 12:00:29.000000000 +0000
> > +++ source/drivers/md/dm.c	2002-12-11 12:00:34.000000000 +0000
> > @@ -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))
>
> 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().

-- 
Kevin Corry
[e-mail]
http://evms.sourceforge.net/