From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2CB51C43334 for ; Tue, 5 Jul 2022 14:23:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232396AbiGEOXv (ORCPT ); Tue, 5 Jul 2022 10:23:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49372 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233067AbiGEOXm (ORCPT ); Tue, 5 Jul 2022 10:23:42 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 718D7C58; Tue, 5 Jul 2022 07:23:41 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 1A887229AA; Tue, 5 Jul 2022 14:23:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1657031020; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=jcuDAT42MRN08+ai7zVZVZ2mWppuivEezCE/Bu5mhtU=; b=owi3RO93Ow5n5L73HHdTiQn2hfqPSvdUleh8KgUxZseP/ZBphU+H2KIcgWp0yOuyd/tPNX tA91jLVfbnO4kHORFIEqLMH2ak5lnHc1iKigU1DIvSOghhPNyDORiB3mSrrCg0C5fUSqJp ynPIt+3GTWOOn7TKZHxuFbmAfJ6ma0w= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1657031020; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=jcuDAT42MRN08+ai7zVZVZ2mWppuivEezCE/Bu5mhtU=; b=T6AyjA5QRILuUAb7gk4isa/t+rjA2Qg9uB5wx/oL5bTShCnXOn0HwI9w9azt/RA5Dsn0N5 jvH/ojhAEBRt/8Aw== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 095FC1339A; Tue, 5 Jul 2022 14:23:37 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id MHsBCWlJxGI7fQAAMHmgww (envelope-from ); Tue, 05 Jul 2022 14:23:37 +0000 Date: Tue, 5 Jul 2022 09:23:34 -0500 From: Goldwyn Rodrigues To: Josef Bacik Cc: Jens Axboe , "Darrick J. Wong" , Al Viro , Stefan Roesch , io-uring@vger.kernel.org, kernel-team@fb.com, linux-mm@kvack.org, linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, david@fromorbit.com, jack@suse.cz, hch@infradead.org, Christoph Hellwig Subject: Re: [PATCH v7 15/15] xfs: Add async buffered write support Message-ID: <20220705142243.hlevsj4pfpahjcdv@fiona> References: <20220601210141.3773402-1-shr@fb.com> <20220601210141.3773402-16-shr@fb.com> <0a75a0c4-e2e5-b403-27bc-e43872fecdc1@kernel.dk> <47dd9e6a-4e08-e562-12ff-5450fc42da77@kernel.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: io-uring@vger.kernel.org On 9:47 05/07, Josef Bacik wrote: > On Fri, Jul 01, 2022 at 12:14:41PM -0600, Jens Axboe wrote: > > On 7/1/22 12:05 PM, Darrick J. Wong wrote: > > > On Fri, Jul 01, 2022 at 08:38:07AM -0600, Jens Axboe wrote: > > >> On 7/1/22 8:30 AM, Jens Axboe wrote: > > >>> On 7/1/22 8:19 AM, Jens Axboe wrote: > > >>>> On 6/30/22 10:39 PM, Al Viro wrote: > > >>>>> On Wed, Jun 01, 2022 at 02:01:41PM -0700, Stefan Roesch wrote: > > >>>>>> This adds the async buffered write support to XFS. For async buffered > > >>>>>> write requests, the request will return -EAGAIN if the ilock cannot be > > >>>>>> obtained immediately. > > >>>>> > > >>>>> breaks generic/471... > > >>>> > > >>>> That test case is odd, because it makes some weird assumptions about > > >>>> what RWF_NOWAIT means. Most notably that it makes it mean if we should > > >>>> instantiate blocks or not. Where did those assumed semantics come from? > > >>>> On the read side, we have clearly documented that it should "not wait > > >>>> for data which is not immediately available". > > >>>> > > >>>> Now it is possible that we're returning a spurious -EAGAIN here when we > > >>>> should not be. And that would be a bug imho. I'll dig in and see what's > > >>>> going on. > > >>> > > >>> This is the timestamp update that needs doing which will now return > > >>> -EAGAIN if IOCB_NOWAIT is set as it may block. > > >>> > > >>> I do wonder if we should just allow inode time updates with IOCB_NOWAIT, > > >>> even on the io_uring side. Either that, or passed in RWF_NOWAIT > > >>> semantics don't map completely to internal IOCB_NOWAIT semantics. At > > >>> least in terms of what generic/471 is doing, but I'm not sure who came > > >>> up with that and if it's established semantics or just some made up ones > > >>> from whomever wrote that test. I don't think they make any sense, to be > > >>> honest. > > >> > > >> Further support that generic/471 is just randomly made up semantics, > > >> it needs to special case btrfs with nocow or you'd get -EAGAIN anyway > > >> for that test. > > >> > > >> And it's relying on some random timing to see if this works. I really > > >> think that test case is just hot garbage, and doesn't test anything > > >> meaningful. > > > > > > I had thought that NOWAIT means "don't wait for *any*thing", > > > which would include timestamp updates... but then I've never been all > > > that clear on what specifically NOWAIT will and won't wait for. :/ > > > > Agree, at least the read semantics (kind of) make sense, but the ones > > seemingly made up by generic/471 don't seem to make any sense at all. > > > > Added Goldwyn to the CC list for this. > > This appears to be just a confusion about what we think NOWAIT should mean. > Looking at the btrfs code it seems like Goldwyn took it as literally as possible > so we wouldn't do any NOWAIT IO's unless it was into a NOCOW area, meaning we > literally wouldn't do anything other than wrap the bio up and fire it off. When I introduced NOWAIT, it was only meant for writes and for those which would block on block allocations or locking. Over time it was included for buffered reads as well. > > The general consensus seems to be that NOWAIT isn't that strict, and that > BTRFS's definition was too strict. I wrote initial patches to give to Stefan to > clean up the Btrfs side to allow us to use NOWAIT under a lot more > circumstances. BTRFS COW path would allocate blocks and the reason it returns -EAGAIN. > > Goldwyn, this test seems to be a little specific to our case, and can be flakey > if the timing isn't just right. I think we should just remove it? Especially > since how we define NOWAIT isn't quite right. Does that sound reasonable to > you? Yes, I agree. It was based on the initial definition and now can be removed. -- Goldwyn