Re: dm thin: optimize away writing all zeroes to unprovisioned blocks

[Mike Snitzer <snitzer@redhat.com>]

On Thu, Dec 04 2014 at 10:33am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:
 
> > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> > index fc9c848..71dd545 100644
> > --- a/drivers/md/dm-thin.c
> > +++ b/drivers/md/dm-thin.c
> > @@ -1230,6 +1230,42 @@ static void process_shared_bio(struct thin_c *tc, struct bio *bio,
> >  	}
> >  }
> 
> A helper like this really belongs in block/bio.c:
> 
> > +/* return true if bio data contains all 0x00's */
> > +bool bio_all_zeros(struct bio *bio) +{
> > +	unsigned long flags;
> > +	struct bio_vec bv;
> > +	struct bvec_iter iter;
> > +
> > +	char *data;
> > +	uint64_t *p;
> > +	int i, count;
> > + +	bool allzeros = true;
> > +
> > +	bio_for_each_segment(bv, bio, iter) {
> > +		data = bvec_kmap_irq(&bv, &flags);
> > +
> > +		p = (uint64_t*)data;
> > +		count = bv.bv_len / sizeof(uint64_t);
> 
> Addressing a bio's contents in terms of uint64_t has the potential to
> access beyond bv.bv_len (byte addressing vs 64bit addressing).  I can
> see you were just looking to be more efficient about checking the bios'
> contents but I'm not convinced it would always be safe.

Actually, given your: count = bv.bv_len / sizeof(uint64_t);

My immediate concern was it would've truncated any partial contents of
the bio_vec beyond the last uint64_t boundary.  Leading to possible
false positive return from the function (because some contents weren't
checked).

--
lvm-devel mailing list
lvm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/lvm-devel