[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dm-devel
Subject:    [dm-devel] Re: [PATCH 2/9] device-mapper log bitset: fix endian
From:       Alasdair G Kergon <agk () redhat ! com>
Date:       2006-01-23 21:09:01
Message-ID: 20060123210901.GL4280 () agk ! surrey ! redhat ! com
[Download RAW message or body]

On Sun, Jan 22, 2006 at 09:37:41PM -0800, Andrew Morton wrote:
> Alasdair G Kergon <agk@redhat.com> wrote:
> >
> >  -	set_bit(bit, (unsigned long *) bs);
> >  +	ext2_set_bit(bit, (unsigned long *) bs);
 
> ext2_set_bit() is non-atomic, so the above code must provide its own
> locking against other CPUs (and threads, if preempt) performing
> modification of this memory.
> 
> Is such locking present?  If not, we should use ext2_set_bit_atomic(). 
> (And if so, the old code could have used __set_bit)

As far as I can tell, all the sets and clears happen from the same 
single-threaded workqueue, but one mirror_map() could run alongside:

        r = ms->rh.log->type->in_sync(ms->rh.log,
                                      bio_to_region(&ms->rh, bio), 0);

which uses:
        ext2_test_bit(bit, (unsigned long *) bs) ? 1 : 0;

So far I haven't found any races here that would cause problems.
(And if there are any, they're probably wider than just making
those operations atomic.)

And it also looks like this code doesn't handle barriers correctly...

Alasdair
-- 
agk@redhat.com


[prev in list] [next in list] [prev in thread] [next in thread]