Messages in this thread | | | From | Jeff Moyer <> | Subject | Re: [PATCH] fs: direct-io: increase bio refcount as batch | Date | Thu, 02 Apr 2015 09:39:39 -0400 |
| |
Ming Lei <ming.lei@canonical.com> writes:
> Hi, > > On Tue, Mar 31, 2015 at 10:02 PM, Jeff Moyer <jmoyer@redhat.com> wrote: >> Ming Lei <ming.lei@canonical.com> writes: >> >>> Each bio is always submitted to block device one by one, >>> so it isn't necessary to increase the bio refcount by one >>> each time with holding dio->bio_lock. >> >> This patch opens up a race where a completion event can come in before >> the refcount for the dio is incremented, resulting in refcount going >> negative. I don't think that will actually cause problems, but it >> certainly is ugly, and I doubt it was the intended design. > > Could you explain why you think it is a race and a bug? When > dio->refcount is negative, dio_bio_end_*() only completes the > current BIO, which is just what the function should do, isn't it?
I didn't say it was a bug. :) Refcounts going negative isn't something that seems clean, though. If you're going to push this patch through, at least add a comment saying that this can happen by design, and is safe.
>> Before I dig into this any further, would you care to comment on why you >> went down this path? Did you see spinlock contention here? And was >> there a resultant performance improvement for some benchmark with the >> patch applied? > > It is just a minor optimization in theory, especially in case of lots of BIO > in one dio.
It seems plausible that it would be a win. It sure would be nice to have some numbers, though.
Cheers, Jeff
| |