Re: [PATCH 2/6] dm: introduce dm_accept_partial_bio

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Mar 17 2014 at  1:08pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 17 Mar 2014, Mike Snitzer wrote:
> 
> > > +void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)
> > > +{
> > > +	struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> > > +	unsigned bi_size = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> > > +	BUG_ON(bi_size > *tio->len_ptr);
> > > +	BUG_ON(n_sectors > bi_size);
> > > +	*tio->len_ptr -= bi_size - n_sectors;
> > > +	bio->bi_iter.bi_size = n_sectors << SECTOR_SHIFT;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
> > > +
> > 
> > The dm_accept_partial_bio interface should return an error (e.g. if bio
> > has REQ_FLUSH set, or the above BUG_ON conditions are met).  The method
> > should probably also be designated with __must_check.  And it should
> > _not_ BUG_ON.  BUG_ON is a crutch that I do not appreciate seeing in new
> > code.  If you'd like to keep them, and get stacktraces, I'd prefer
> > seeing them converted to WARN_ON that is followed with an error return.
> 
> You need to return error for externally triggered events (like disk 
> errors, memory allocation failures, etc.), not for bugs in the code.
> 
> Here, if someone passes invalid n_sectors, it is not externally triggered 
> event (the code that passed the invalid argument is just buggy) - so 
> crashing on BUG_ON is OK.
> 
> 
> Another problem is that warnings can be easily missed, BUG won't be 
> missed. For example, the lvm testsuite generates large number of messages 
> to the syslog. Consequently, when I run the testsuite, I don't read 
> syslog. Consequently, if you convert BUG_ON to WARN_ON and the testsuite 
> triggers it, I will miss it.
> 
> 
> Regarding REQ_FLUSH - it would already crash on NULL pointer dereference 
> when accessing *tio->len_ptr.

None of what you responded with here is acceptable.  Your preferred
conventions don't accomodate people implementing a new target, or
adapting an existing target, to use this new interface you've proposed.
In this instance, BUG() is a hostile response to what is likely a minor
oversight.

Returning an error will not allow the caller to do what they thought
would happen.  So even you should catch that (despite you apparently not
looking at the kernel log messages).