Messages in this thread | | | Date | Wed, 8 Aug 2012 23:00:19 -0700 | From | Tejun Heo <> | Subject | Re: [PATCH v5 03/12] block: Add bio_reset() |
| |
Hello,
On Wed, Aug 08, 2012 at 05:07:11PM -0700, Kent Overstreet wrote: > > > +void bio_reset(struct bio *bio) > > > +{ > > > + unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS); > > > > How many flags are we talking about? If there aren't too many, I'd > > prefer explicit BIO_FLAGS_PRESERVED or whatever. > > It mostly isn't actual flags that are preserved - the high bits of the > flags are used for indicating what slab the bvec was allocated from, and > that's the main thing that has to be preserved. > > So that's why I went with defining the things that are reset instead of > the things that are preserved. > > I would prefer if bitfields were used for at least BIO_POOL_IDX, but the > problem is flags is used as an atomic bit vector for BIO_UPTODATE. > > But flags isn't treated as an atomic bit vector elsewhere - > bio_flagged() doesn't use test_bit(), and flags are set/cleared with
Not using test_bit() may not be necessarily wrong.
> atomic bit operations in some places but not in others (probably _most_ > of them are technically safe, but... ick).
Mixing atomic and non-atomic modifications is almost always wrong tho.
Anyways, understood. Can you *please* put some comment what bits are being preserved across reset then? Things like this aren't obvious at all and need ample explanation.
Thanks.
-- tejun
| |