From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753416AbbDBNjo (ORCPT ); Thu, 2 Apr 2015 09:39:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45921 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752595AbbDBNjn (ORCPT ); Thu, 2 Apr 2015 09:39:43 -0400 From: Jeff Moyer To: Ming Lei Cc: Alexander Viro , Linux FS Devel , Linux Kernel Mailing List Subject: Re: [PATCH] fs: direct-io: increase bio refcount as batch References: <1427802746-30432-1-git-send-email-ming.lei@canonical.com> X-PGP-KeyID: 1F78E1B4 X-PGP-CertKey: F6FE 280D 8293 F72C 65FD 5A58 1FF8 A7CA 1F78 E1B4 X-PCLoadLetter: What the f**k does that mean? Date: Thu, 02 Apr 2015 09:39:39 -0400 In-Reply-To: (Ming Lei's message of "Wed, 1 Apr 2015 10:04:44 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ming Lei writes: > Hi, > > On Tue, Mar 31, 2015 at 10:02 PM, Jeff Moyer wrote: >> Ming Lei 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