From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753118AbdARQCF (ORCPT ); Wed, 18 Jan 2017 11:02:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56372 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752390AbdARQCC (ORCPT ); Wed, 18 Jan 2017 11:02:02 -0500 From: Jeff Moyer To: Jens Axboe Cc: linux-block@vger.kernel.org, agk@redhat.com, snitzer@redhat.com, dm-devel@redhat.com, shli@kernel.org, linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, hch@lst.de Subject: Re: [patch] block: add blktrace C events for bio-based drivers References: 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: Wed, 18 Jan 2017 11:01:46 -0500 In-Reply-To: (Jens Axboe's message of "Tue, 17 Jan 2017 14:07:32 -0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 18 Jan 2017 16:01:48 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Jens, Jens Axboe writes: > I like the change, hate the naming. I'd prefer one of two things: > > - Add bio_endio_complete() instead. That name sucks too, the > important part is flipping the __name() to have a trace > version instead. ITYM a notrace version. By default, we want tracing for bio_endio. The only callers that need the inverse of that are in the request-based path, and there are only 2 of them. > - Mark the bio as trace completed, and keep the naming. Since > it's only off the completion path, that can be just marking > the bi_flags non-atomically. One issue with this is in generic_make_request_checks, where we can call bio_endio without having called trace_block_bio_queue (so you could get a C event with no corresponding Q). To address that, we could make the flag indicate that trace_block_bio_queue was performed, and clear it in bio_complete, like so: if (test_and_clear_bit(BIO_QUEUE_TRACED, &bio->bi_flags)) trace_block_bio_complete(...); That would solve the problem of duplicate completions, but requires setting the flag in the submission path and clearing it in the completion path. I think the former can be done with just a bio_set_flag (i.e. non-atomic), right? Of course, where to stick that bio_set_flag call is another bike-shedding discussion waiting to happen (i.e. does it go in the tracepoint itself?). Alternatively, we could set the trace_completed flag in the paths where we end I/O without having done the trace_block_bio_queue, but that seems way uglier to me. Can you think of any other options? If we're choosing from the above, my preference is for adding the bio_endio_notrace(), since it's so much simpler. Cheers, Jeff