Ming Lei <ming.lei@xxxxxxxxxxxxx> writes: > Hi, > > On Tue, Mar 31, 2015 at 10:02 PM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote: >> Ming Lei <ming.lei@xxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
