Re: [PATCH 3/3] xfs: alignment check bio buffers

Christoph Hellwig
2019-08-21T23:30:43Z

On Wed, Aug 21, 2019 at 09:39:04AM -0400, Brian Foster wrote:
> > @@ -36,9 +57,12 @@ xfs_rw_bdev(
> > unsigned int off = offset_in_page(data);
> > unsigned int len = min_t(unsigned, left, PAGE_SIZE - off);
> >
> > - while (bio_add_page(bio, page, len, off) != len) {
> > + while ((ret = xfs_bio_add_page(bio, page, len, off)) != len) {
> > struct bio *prev = bio;
> >
> > + if (ret < 0)
> > + goto submit;
> > +
>
> Hmm.. is submitting the bio really the right thing to do if we get here
> and have failed to add any pages to the bio? If we're already seeing
> weird behavior for bios with unaligned data memory, this seems like a
> recipe for similar weirdness. We'd also end up doing a partial write in
> scenarios where we already know we're returning an error. Perhaps we
> should create an error path or use a check similar to what is already in
> xfs_buf_ioapply_map() (though I'm not a fan of submitting a partial I/O
> when we already know we're going to return an error) to call bio_endio()
> to undo any chaining.

It is not the right thing to do. Calling bio_endio after setting
an error is the right thing to do (modulo any other cleanup needed).