From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9A9BE381 for ; Mon, 4 Sep 2023 01:02:53 +0000 (UTC) Received: from mail-pl1-x631.google.com (mail-pl1-x631.google.com [IPv6:2607:f8b0:4864:20::631]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C5DF61AB for ; Sun, 3 Sep 2023 18:02:36 -0700 (PDT) Received: by mail-pl1-x631.google.com with SMTP id d9443c01a7336-1bf7a6509deso1973775ad.3 for ; Sun, 03 Sep 2023 18:02:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1693789356; x=1694394156; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=qC164uBpiTqMDgXCvRgiQKt4Gh699N5mZOL/vYt7en0Jbwz76nMzEY83UKBVSQIW6l aeb79T1fQrqEYwAnWqEg0BUEVogsLmyDn7Er7QB5MMmb3evyGFkJ31LnyaFbCd66DWcw 2rEqHGNd94yLw2873vPlsxfffzMVRPFxi+gQvuhDp/nt+kHeFVOC7RU50NpTqz237x8d 0EgVDl7W9L5GKWAUtPLGkVf3nghZNwTKtocs80JusMNZMyy2xNrwPVPtVOWAc9Ov7Ec+ l+Jy4wlPaRYKxlaBf+eoyU6yz7sgMqtArlqAkIUtWnDR1tDFWMVYywwL0pwAGBUewBHm OWuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693789356; x=1694394156; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=csCBZ4advq9dK980lKNpws3oVSrxk51QPW5uHxGR8qJ11WIqqi+89xRjtP0ayKPUx+ NO7mGUWH2eIrJ7QnkI1vuvE3YrjHXIbGsKgsgTxi8NmwqP4kSr8J4xhC8m4Sm1ua8owT Z3wFIlxJ8M1iWzQu1KEO5eiaTI8elYK3EDXZgWl2kK4qnDjL4G7uwjrrAOXmlrPISiHD SuK9QADi+3KY/g0MtK6EwyjOIVDORZzfyZABB3rZcBqiElkSVH9izyczPQjp0Zber+x3 60eqTQe3StpR2SLpxNgL2Am0/9JrKLSwhn4IDRxXIT5qRFn/nxnWQmdATi1RNPKGQxlR ytUg== X-Gm-Message-State: AOJu0YyfZMad7dV1MJ1kAXvG0ldx7EobYUfdGWpu4euBJRLQHfALPI57 NQ371aFpTvImDR9BokYKKIGjRw== X-Google-Smtp-Source: AGHT+IGGrky2P+bBuH4AKgWBeYdqXOP6u+qaOJyeKMBTbWb7BRdsdnk0jjxmGVwySX0sZe8eaRNDcw== X-Received: by 2002:a17:902:ecc8:b0:1c1:fe97:bf34 with SMTP id a8-20020a170902ecc800b001c1fe97bf34mr8040994plh.24.1693789355853; Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dread.disaster.area (pa49-195-66-88.pa.nsw.optusnet.com.au. [49.195.66.88]) by smtp.gmail.com with ESMTPSA id d4-20020a170902c18400b001bdcafcf8d3sm6351806pld.69.2023.09.03.18.02.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qcxz6-00AVA9-2L; Mon, 04 Sep 2023 11:02:32 +1000 Date: Mon, 4 Sep 2023 11:02:32 +1000 From: Dave Chinner To: Hao Xu Cc: io-uring@vger.kernel.org, Jens Axboe , Dominique Martinet , Pavel Begunkov , Christian Brauner , Alexander Viro , Stefan Roesch , Clay Harris , "Darrick J . Wong" , linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-cachefs@redhat.com, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-unionfs@vger.kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, linux-btrfs@vger.kernel.org, codalist@coda.cs.cmu.edu, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com, linux-mm@kvack.org, linux-nilfs@vger.kernel.org, devel@lists.orangefs.org, linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, linux-mtd@lists.infradead.org, Wanpeng Li Subject: Re: [PATCH 02/11] xfs: add NOWAIT semantics for readdir Message-ID: References: <20230827132835.1373581-1-hao.xu@linux.dev> <20230827132835.1373581-3-hao.xu@linux.dev> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230827132835.1373581-3-hao.xu@linux.dev> X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net On Sun, Aug 27, 2023 at 09:28:26PM +0800, Hao Xu wrote: > From: Hao Xu > > Implement NOWAIT semantics for readdir. Return EAGAIN error to the > caller if it would block, like failing to get locks, or going to > do IO. > > Co-developed-by: Dave Chinner Not really. "Co-developed" implies equal development input between all the parties, which is not the case here - this patch is based on prototype I wrote, whilst you're doing the refining, testing and correctness work. In these cases with XFS code, we add a line in the commit message to say: "This is based on a patch originally written by Dave Chinner." > Signed-off-by: Dave Chinner > Signed-off-by: Hao Xu > [fixes deadlock issue, tweak code style] With a signoff chain like you already have. In the end you'll also get a RVB from me, which seems rather wrong to me if I've apparently been "co-developing" the code.... .... > @@ -156,7 +157,9 @@ xfs_dir2_block_getdents( > if (xfs_dir2_dataptr_to_db(geo, ctx->pos) > geo->datablk) > return 0; > > - error = xfs_dir3_block_read(args->trans, dp, &bp); > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) > + flags |= XFS_DABUF_NOWAIT; > + error = xfs_dir3_block_read(args->trans, dp, flags, &bp); > if (error) > return error; > Given we do this same check in both block and leaf formats to set XFS_DABUF_NOWAIT, and we do the DIR_CONTEXT_F_NOWAIT check in xfs_readdir() as well, we should probably do this check once at the higher level and pass flags down from there with XFS_DABUF_NOWAIT already set. > @@ -240,6 +243,7 @@ xfs_dir2_block_getdents( > STATIC int > xfs_dir2_leaf_readbuf( > struct xfs_da_args *args, > + struct dir_context *ctx, > size_t bufsize, > xfs_dir2_off_t *cur_off, > xfs_dablk_t *ra_blk, > @@ -258,10 +262,15 @@ xfs_dir2_leaf_readbuf( > struct xfs_iext_cursor icur; > int ra_want; > int error = 0; > - > - error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > - if (error) > - goto out; > + unsigned int flags = 0; > + > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { > + flags |= XFS_DABUF_NOWAIT; > + } else { > + error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > + if (error) > + goto out; > + } Especially as, in hindsight, this doesn't make a whole lot of sense. If XFS_DABUF_NOWAIT is set, we keep going until xfs_ilock_data_map_shared_nowait() where we call xfs_need_iread_extents() to see if we need to read the extents in and abort at that point. So, really, we shouldn't get this far with nowait semantics if we haven't read the extents in yet - we're supposed to already have the inode locked here and so we should have already checked this condition before we bother locking the inode... i.e. all we should be doing here is this: if (!(flags & XFS_DABUF_NOWAIT)) { error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); if (error) goto out; } And then we don't need to pass the VFS dir_context down into low level XFS functions unnecessarily. > > /* > * Look for mapped directory blocks at or above the current offset. > @@ -280,7 +289,7 @@ xfs_dir2_leaf_readbuf( > new_off = xfs_dir2_da_to_byte(geo, map.br_startoff); > if (new_off > *cur_off) > *cur_off = new_off; > - error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, 0, &bp); > + error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, flags, &bp); > if (error) > goto out; > > @@ -360,6 +369,7 @@ xfs_dir2_leaf_getdents( > int byteoff; /* offset in current block */ > unsigned int offset = 0; > int error = 0; /* error return value */ > + int written = 0; > > /* > * If the offset is at or past the largest allowed value, > @@ -391,10 +401,17 @@ xfs_dir2_leaf_getdents( > bp = NULL; > } > > - if (*lock_mode == 0) > - *lock_mode = xfs_ilock_data_map_shared(dp); > - error = xfs_dir2_leaf_readbuf(args, bufsize, &curoff, > - &rablk, &bp); > + if (*lock_mode == 0) { > + *lock_mode = > + xfs_ilock_data_map_shared_generic(dp, > + ctx->flags & DIR_CONTEXT_F_NOWAIT); > + if (!*lock_mode) { > + error = -EAGAIN; > + break; > + } > + } > + error = xfs_dir2_leaf_readbuf(args, ctx, bufsize, > + &curoff, &rablk, &bp); int xfs_ilock_readdir( struct xfs_inode *ip, int flags) { if (flags & XFS_DABUF_NOWAIT) { if (!xfs_ilock_nowait(dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(dp); } And then this code simply becomes: if (*lock_mode == 0) *lock_mode = xfs_ilock_readdir(ip, flags); > if (error || !bp) > break; > > @@ -479,6 +496,7 @@ xfs_dir2_leaf_getdents( > */ > offset += length; > curoff += length; > + written += length; > /* bufsize may have just been a guess; don't go negative */ > bufsize = bufsize > length ? bufsize - length : 0; > } > @@ -492,6 +510,8 @@ xfs_dir2_leaf_getdents( > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > if (bp) > xfs_trans_brelse(args->trans, bp); > + if (error == -EAGAIN && written > 0) > + error = 0; > return error; > } > > @@ -514,6 +534,7 @@ xfs_readdir( > unsigned int lock_mode; > bool isblock; > int error; > + bool nowait; > > trace_xfs_readdir(dp); > > @@ -531,7 +552,11 @@ xfs_readdir( > if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) > return xfs_dir2_sf_getdents(&args, ctx); > > - lock_mode = xfs_ilock_data_map_shared(dp); > + nowait = ctx->flags & DIR_CONTEXT_F_NOWAIT; > + lock_mode = xfs_ilock_data_map_shared_generic(dp, nowait); > + if (!lock_mode) > + return -EAGAIN; > + Given what I said above: if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { /* * If we need to read extents, then we must do IO * and we must use exclusive locking. We don't want * to do either of those things, so just bail if we * have to read extents. Doing this check explicitly * here means we don't have to do it anywhere else * in the XFS_DABUF_NOWAIT path. */ if (xfs_need_iread_extents(&ip->i_df)) return -EAGAIN; flags |= XFS_DABUF_NOWAIT; } lock_mode = xfs_ilock_readdir(dp, flags); And with this change, we probably should be marking the entire operation as having nowait semantics. i.e. using args->op_flags here and only use XFS_DABUF_NOWAIT for the actual IO. ie. args->op_flags |= XFS_DA_OP_NOWAIT; This makes it clear that the entire directory op should run under NOWAIT constraints, and it avoids needing to pass an extra flag through the stack. That then makes the readdir locking function look like this: /* * When we are locking an inode for readdir, we need to ensure that * the extents have been read in first. This requires the inode to * be locked exclusively across the extent read, but otherwise we * want to use shared locking. * * For XFS_DA_OP_NOWAIT operations, we do an up-front check to see * if the extents have been read in, so all we need to do in this * case is a shared try-lock as we never need exclusive locking in * this path. */ static int xfs_ilock_readdir( struct xfs_da_args *args) { if (args->op_flags & XFS_DA_OP_NOWAIT) { if (!xfs_ilock_nowait(args->dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(args->dp); } > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 9e62cc500140..d088f7d0c23a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -120,6 +120,33 @@ xfs_ilock_data_map_shared( > return lock_mode; > } > > +/* > + * Similar to xfs_ilock_data_map_shared(), except that it will only try to lock > + * the inode in shared mode if the extents are already in memory. If it fails to > + * get the lock or has to do IO to read the extent list, fail the operation by > + * returning 0 as the lock mode. > + */ > +uint > +xfs_ilock_data_map_shared_nowait( > + struct xfs_inode *ip) > +{ > + if (xfs_need_iread_extents(&ip->i_df)) > + return 0; > + if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED)) > + return 0; > + return XFS_ILOCK_SHARED; > +} > + > +int > +xfs_ilock_data_map_shared_generic( > + struct xfs_inode *dp, > + bool nowait) > +{ > + if (nowait) > + return xfs_ilock_data_map_shared_nowait(dp); > + return xfs_ilock_data_map_shared(dp); > +} And all this "generic" locking stuff goes away. FWIW, IMO, "generic" is a poor name for an XFS function as there's nothing "generic" in XFS. We tend name the functions after what they do, not some abstract concept. Leave "generic" as a keyword for widely used core infrastructure functions, not niche, one-off use cases like this. Cheers, Dave. -- Dave Chinner david@fromorbit.com 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 us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3E50FC71153 for ; Mon, 4 Sep 2023 01:02:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1693789365; h=from:from:sender:sender:reply-to:subject:subject: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:list-id:list-help:list-unsubscribe: list-subscribe:list-post; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=IpMme351RVAzAXF1hYfi9+gH72euasEgg3PwyrofRlMvd1VU5sjsEp/dDRdRwlJEoZcRRu 3rLx2hyMAyp3WpUwKqlcXAw7nlWRJlPU5IHjCgS8XVRgf43/aSmv27C8eyAF7EX4EsQenR S93794cakUZTOh/7tPaguluif7hfT94= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-355-xmaSJZKKMkS5ut5wjkdhiA-1; Sun, 03 Sep 2023 21:02:42 -0400 X-MC-Unique: xmaSJZKKMkS5ut5wjkdhiA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 46F21101A529; Mon, 4 Sep 2023 01:02:41 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com [10.30.29.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9C14F141D960; Mon, 4 Sep 2023 01:02:40 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (localhost [IPv6:::1]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 613031946595; Mon, 4 Sep 2023 01:02:40 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 2EBAA1946588 for ; Mon, 4 Sep 2023 01:02:39 +0000 (UTC) Received: by smtp.corp.redhat.com (Postfix) id 1C883200BABC; Mon, 4 Sep 2023 01:02:39 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast02.extmail.prod.ext.rdu2.redhat.com [10.11.55.18]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 14C07200A86A for ; Mon, 4 Sep 2023 01:02:39 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E999F805BFB for ; Mon, 4 Sep 2023 01:02:38 +0000 (UTC) Received: from mail-pl1-f178.google.com (mail-pl1-f178.google.com [209.85.214.178]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-654-UuwdsSUcNeCjxTcqKq6x1w-1; Sun, 03 Sep 2023 21:02:37 -0400 X-MC-Unique: UuwdsSUcNeCjxTcqKq6x1w-1 Received: by mail-pl1-f178.google.com with SMTP id d9443c01a7336-1bf48546ccfso1982545ad.2 for ; Sun, 03 Sep 2023 18:02:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693789356; x=1694394156; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=kPcPMWchwOvwc/mQYxTKmyEQZQKMPK3FCJ2A969SSOO4+4FH3FHqDoAy+nkGf9SxHh Y7/VvhR/sHmi40BjVQ71iRdObCwxxDJa4ZYF0IhXDMVp9yldeNe+DNFvrn9u/LK4SIwI PPpFTx+QHFroqDpsoQiFDWFHJ1Nf4Kb9nO3YBn/j2BJuvn3ix/UPVpeTzNoPkTEty00s IRIIMioJs8WwUqobouHaoX04+MQJVXlhBthtbaW+Erg2goUAaJlNzMNsWqOFGXKMjjih 1jeAXrbNgmWFQdtO2PEU1p4aV7HdPD4Y7hyA/lxiFy25nSk/D/gEbcInisKAnH1Yc4Eo zOUw== X-Gm-Message-State: AOJu0YwAmcxDfOEt4KDT0yyk4cdPp1QioQdXLExIxtvrZBLA+nuJKDp3 glWSX9ZuzatoyIP1ZgP+VMIY7A== X-Google-Smtp-Source: AGHT+IGGrky2P+bBuH4AKgWBeYdqXOP6u+qaOJyeKMBTbWb7BRdsdnk0jjxmGVwySX0sZe8eaRNDcw== X-Received: by 2002:a17:902:ecc8:b0:1c1:fe97:bf34 with SMTP id a8-20020a170902ecc800b001c1fe97bf34mr8040994plh.24.1693789355853; Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dread.disaster.area (pa49-195-66-88.pa.nsw.optusnet.com.au. [49.195.66.88]) by smtp.gmail.com with ESMTPSA id d4-20020a170902c18400b001bdcafcf8d3sm6351806pld.69.2023.09.03.18.02.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qcxz6-00AVA9-2L; Mon, 04 Sep 2023 11:02:32 +1000 Date: Mon, 4 Sep 2023 11:02:32 +1000 From: Dave Chinner To: Hao Xu Message-ID: References: <20230827132835.1373581-1-hao.xu@linux.dev> <20230827132835.1373581-3-hao.xu@linux.dev> MIME-Version: 1.0 In-Reply-To: <20230827132835.1373581-3-hao.xu@linux.dev> X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 Subject: Re: [Cluster-devel] [PATCH 02/11] xfs: add NOWAIT semantics for readdir X-BeenThere: cluster-devel@redhat.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: "\[Cluster devel\]" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Wanpeng Li , "Darrick J . Wong" , Dominique Martinet , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Stefan Roesch , Clay Harris , linux-s390@vger.kernel.org, linux-nilfs@vger.kernel.org, codalist@coda.cs.cmu.edu, cluster-devel@redhat.com, linux-cachefs@redhat.com, linux-ext4@vger.kernel.org, devel@lists.orangefs.org, linux-cifs@vger.kernel.org, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-block@vger.kernel.org, Alexander Viro , io-uring@vger.kernel.org, Jens Axboe , Christian Brauner , netdev@vger.kernel.org, samba-technical@lists.samba.org, linux-unionfs@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mtd@lists.infradead.org, bpf@vger.kernel.org, Pavel Begunkov , linux-btrfs@vger.kernel.org Errors-To: cluster-devel-bounces@redhat.com Sender: "Cluster-devel" X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: fromorbit.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Aug 27, 2023 at 09:28:26PM +0800, Hao Xu wrote: > From: Hao Xu > > Implement NOWAIT semantics for readdir. Return EAGAIN error to the > caller if it would block, like failing to get locks, or going to > do IO. > > Co-developed-by: Dave Chinner Not really. "Co-developed" implies equal development input between all the parties, which is not the case here - this patch is based on prototype I wrote, whilst you're doing the refining, testing and correctness work. In these cases with XFS code, we add a line in the commit message to say: "This is based on a patch originally written by Dave Chinner." > Signed-off-by: Dave Chinner > Signed-off-by: Hao Xu > [fixes deadlock issue, tweak code style] With a signoff chain like you already have. In the end you'll also get a RVB from me, which seems rather wrong to me if I've apparently been "co-developing" the code.... .... > @@ -156,7 +157,9 @@ xfs_dir2_block_getdents( > if (xfs_dir2_dataptr_to_db(geo, ctx->pos) > geo->datablk) > return 0; > > - error = xfs_dir3_block_read(args->trans, dp, &bp); > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) > + flags |= XFS_DABUF_NOWAIT; > + error = xfs_dir3_block_read(args->trans, dp, flags, &bp); > if (error) > return error; > Given we do this same check in both block and leaf formats to set XFS_DABUF_NOWAIT, and we do the DIR_CONTEXT_F_NOWAIT check in xfs_readdir() as well, we should probably do this check once at the higher level and pass flags down from there with XFS_DABUF_NOWAIT already set. > @@ -240,6 +243,7 @@ xfs_dir2_block_getdents( > STATIC int > xfs_dir2_leaf_readbuf( > struct xfs_da_args *args, > + struct dir_context *ctx, > size_t bufsize, > xfs_dir2_off_t *cur_off, > xfs_dablk_t *ra_blk, > @@ -258,10 +262,15 @@ xfs_dir2_leaf_readbuf( > struct xfs_iext_cursor icur; > int ra_want; > int error = 0; > - > - error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > - if (error) > - goto out; > + unsigned int flags = 0; > + > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { > + flags |= XFS_DABUF_NOWAIT; > + } else { > + error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > + if (error) > + goto out; > + } Especially as, in hindsight, this doesn't make a whole lot of sense. If XFS_DABUF_NOWAIT is set, we keep going until xfs_ilock_data_map_shared_nowait() where we call xfs_need_iread_extents() to see if we need to read the extents in and abort at that point. So, really, we shouldn't get this far with nowait semantics if we haven't read the extents in yet - we're supposed to already have the inode locked here and so we should have already checked this condition before we bother locking the inode... i.e. all we should be doing here is this: if (!(flags & XFS_DABUF_NOWAIT)) { error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); if (error) goto out; } And then we don't need to pass the VFS dir_context down into low level XFS functions unnecessarily. > > /* > * Look for mapped directory blocks at or above the current offset. > @@ -280,7 +289,7 @@ xfs_dir2_leaf_readbuf( > new_off = xfs_dir2_da_to_byte(geo, map.br_startoff); > if (new_off > *cur_off) > *cur_off = new_off; > - error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, 0, &bp); > + error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, flags, &bp); > if (error) > goto out; > > @@ -360,6 +369,7 @@ xfs_dir2_leaf_getdents( > int byteoff; /* offset in current block */ > unsigned int offset = 0; > int error = 0; /* error return value */ > + int written = 0; > > /* > * If the offset is at or past the largest allowed value, > @@ -391,10 +401,17 @@ xfs_dir2_leaf_getdents( > bp = NULL; > } > > - if (*lock_mode == 0) > - *lock_mode = xfs_ilock_data_map_shared(dp); > - error = xfs_dir2_leaf_readbuf(args, bufsize, &curoff, > - &rablk, &bp); > + if (*lock_mode == 0) { > + *lock_mode = > + xfs_ilock_data_map_shared_generic(dp, > + ctx->flags & DIR_CONTEXT_F_NOWAIT); > + if (!*lock_mode) { > + error = -EAGAIN; > + break; > + } > + } > + error = xfs_dir2_leaf_readbuf(args, ctx, bufsize, > + &curoff, &rablk, &bp); int xfs_ilock_readdir( struct xfs_inode *ip, int flags) { if (flags & XFS_DABUF_NOWAIT) { if (!xfs_ilock_nowait(dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(dp); } And then this code simply becomes: if (*lock_mode == 0) *lock_mode = xfs_ilock_readdir(ip, flags); > if (error || !bp) > break; > > @@ -479,6 +496,7 @@ xfs_dir2_leaf_getdents( > */ > offset += length; > curoff += length; > + written += length; > /* bufsize may have just been a guess; don't go negative */ > bufsize = bufsize > length ? bufsize - length : 0; > } > @@ -492,6 +510,8 @@ xfs_dir2_leaf_getdents( > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > if (bp) > xfs_trans_brelse(args->trans, bp); > + if (error == -EAGAIN && written > 0) > + error = 0; > return error; > } > > @@ -514,6 +534,7 @@ xfs_readdir( > unsigned int lock_mode; > bool isblock; > int error; > + bool nowait; > > trace_xfs_readdir(dp); > > @@ -531,7 +552,11 @@ xfs_readdir( > if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) > return xfs_dir2_sf_getdents(&args, ctx); > > - lock_mode = xfs_ilock_data_map_shared(dp); > + nowait = ctx->flags & DIR_CONTEXT_F_NOWAIT; > + lock_mode = xfs_ilock_data_map_shared_generic(dp, nowait); > + if (!lock_mode) > + return -EAGAIN; > + Given what I said above: if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { /* * If we need to read extents, then we must do IO * and we must use exclusive locking. We don't want * to do either of those things, so just bail if we * have to read extents. Doing this check explicitly * here means we don't have to do it anywhere else * in the XFS_DABUF_NOWAIT path. */ if (xfs_need_iread_extents(&ip->i_df)) return -EAGAIN; flags |= XFS_DABUF_NOWAIT; } lock_mode = xfs_ilock_readdir(dp, flags); And with this change, we probably should be marking the entire operation as having nowait semantics. i.e. using args->op_flags here and only use XFS_DABUF_NOWAIT for the actual IO. ie. args->op_flags |= XFS_DA_OP_NOWAIT; This makes it clear that the entire directory op should run under NOWAIT constraints, and it avoids needing to pass an extra flag through the stack. That then makes the readdir locking function look like this: /* * When we are locking an inode for readdir, we need to ensure that * the extents have been read in first. This requires the inode to * be locked exclusively across the extent read, but otherwise we * want to use shared locking. * * For XFS_DA_OP_NOWAIT operations, we do an up-front check to see * if the extents have been read in, so all we need to do in this * case is a shared try-lock as we never need exclusive locking in * this path. */ static int xfs_ilock_readdir( struct xfs_da_args *args) { if (args->op_flags & XFS_DA_OP_NOWAIT) { if (!xfs_ilock_nowait(args->dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(args->dp); } > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 9e62cc500140..d088f7d0c23a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -120,6 +120,33 @@ xfs_ilock_data_map_shared( > return lock_mode; > } > > +/* > + * Similar to xfs_ilock_data_map_shared(), except that it will only try to lock > + * the inode in shared mode if the extents are already in memory. If it fails to > + * get the lock or has to do IO to read the extent list, fail the operation by > + * returning 0 as the lock mode. > + */ > +uint > +xfs_ilock_data_map_shared_nowait( > + struct xfs_inode *ip) > +{ > + if (xfs_need_iread_extents(&ip->i_df)) > + return 0; > + if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED)) > + return 0; > + return XFS_ILOCK_SHARED; > +} > + > +int > +xfs_ilock_data_map_shared_generic( > + struct xfs_inode *dp, > + bool nowait) > +{ > + if (nowait) > + return xfs_ilock_data_map_shared_nowait(dp); > + return xfs_ilock_data_map_shared(dp); > +} And all this "generic" locking stuff goes away. FWIW, IMO, "generic" is a poor name for an XFS function as there's nothing "generic" in XFS. We tend name the functions after what they do, not some abstract concept. Leave "generic" as a keyword for widely used core infrastructure functions, not niche, one-off use cases like this. Cheers, Dave. -- Dave Chinner david@fromorbit.com 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 lists.sourceforge.net (lists.sourceforge.net [216.105.38.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 83581CA0FE3 for ; Mon, 4 Sep 2023 01:02:49 +0000 (UTC) Received: from [127.0.0.1] (helo=sfs-ml-2.v29.lw.sourceforge.com) by sfs-ml-2.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1qcxzL-0000Kr-Tw; Mon, 04 Sep 2023 01:02:46 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1qcxzK-0000Kf-TY for linux-f2fs-devel@lists.sourceforge.net; Mon, 04 Sep 2023 01:02:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=In-Reply-To:Content-Type:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=F2MaILzLyHSsVn5fjJgN9yv723 wih45nCmCf39FwlVz9W4WTlSR3XI4tnHRZvAzFqc/I75EF5OqS5tl6Z75E7GoPz2S1Lk5649o262x 1dZNlDPflHos98owHRQqbnzQUR31gCRbYW2ppBvX7izCmu1m7TK4Rr6b9d1Z84O3Xg7A=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To :From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=Gaq5km3li1hGzUq6SD7kjs14Qf dyuLVgHhvuGe9ZL/l+TSo/7eT9vpFQWYB09jQAtBynxVi4wsuIIVgN7Sc8WGXFwws6D8c7xFXWFYt 1rmUlXniO1w9uJfGbdUmnRRLMEO6ilXSSjDv+xDyhzoSDht87l1mCOyD5oFO7p6Z8+nY=; Received: from mail-pl1-f171.google.com ([209.85.214.171]) by sfi-mx-1.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.95) id 1qcxzF-0089tY-Jx for linux-f2fs-devel@lists.sourceforge.net; Mon, 04 Sep 2023 01:02:45 +0000 Received: by mail-pl1-f171.google.com with SMTP id d9443c01a7336-1bf48546ccfso1982575ad.2 for ; Sun, 03 Sep 2023 18:02:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1693789356; x=1694394156; darn=lists.sourceforge.net; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=S0OXYTBUpPmoAdGapCzrvh4k4elh5yBYAYupcBYs1W9iQXiDMReVAU2x4IQ57Pc4SG 8UR8g6qjuzjJNQYx+zmK9HdMsIlufofq9Wij8Y28UZeeHIHv08GR1TrxlT9pogMa+LRe LVgni07X7aA/GrVfxDGablqz+pn0ghxaQTolKJS5KleizcO27HKgvVgJNHRpvJGTWDa2 Zd83LxjiZvEANzloNmVCPfK1Y9I8MPVoiQnPZygCtbk1taeEzh/lOorEg20EQ9Be89A6 Vrs7FFocJDWWv8ZZDjL0/stlrEICmLoj/edWppj7G+ZAWq9xhf7GJG8n3Bp6IYhYAoJj J0JQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693789356; x=1694394156; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=MxArW01mAq+kZJIA7yh7/czwX2nw5GtZ74cnqkUFNC0JxrKml8jVCRXZP0N6qPK5un LknewagST2/XuYhZlELfsjSHNqdVch+MBJUYU45pfr1ibbMPXWyFNv5QhYNeP0fymvmY ITje+h5vhMzeXNTEnapAwAJgOdVkTOG1Lg7iBY7qccTXujThvc4jrckkWtb7i/sUcqmX yT1JRgt0uRhcMTuzy1HwC0YuGV3TDXX7myBWISMgLS7v4c5Q6My24vwKcNnQ2ZdOfWVG t/MVdOnravSEMOMvrmvZjtInRw0RrttfeeeQ4KBnketEuafHKCmT9ZNUSICzFuBg7r39 XbDQ== X-Gm-Message-State: AOJu0YypXQX/pj6bsqaW6+KpzFyVgotn/GjRtjW3Z8VVtsM6fFwpqKXQ SvpYAuLg1TEA/+H1IaKYjHObug== X-Google-Smtp-Source: AGHT+IGGrky2P+bBuH4AKgWBeYdqXOP6u+qaOJyeKMBTbWb7BRdsdnk0jjxmGVwySX0sZe8eaRNDcw== X-Received: by 2002:a17:902:ecc8:b0:1c1:fe97:bf34 with SMTP id a8-20020a170902ecc800b001c1fe97bf34mr8040994plh.24.1693789355853; Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dread.disaster.area (pa49-195-66-88.pa.nsw.optusnet.com.au. [49.195.66.88]) by smtp.gmail.com with ESMTPSA id d4-20020a170902c18400b001bdcafcf8d3sm6351806pld.69.2023.09.03.18.02.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qcxz6-00AVA9-2L; Mon, 04 Sep 2023 11:02:32 +1000 Date: Mon, 4 Sep 2023 11:02:32 +1000 To: Hao Xu Message-ID: References: <20230827132835.1373581-1-hao.xu@linux.dev> <20230827132835.1373581-3-hao.xu@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230827132835.1373581-3-hao.xu@linux.dev> X-Headers-End: 1qcxzF-0089tY-Jx Subject: Re: [f2fs-dev] [PATCH 02/11] xfs: add NOWAIT semantics for readdir X-BeenThere: linux-f2fs-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Dave Chinner via Linux-f2fs-devel Reply-To: Dave Chinner Cc: Wanpeng Li , "Darrick J . Wong" , Dominique Martinet , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Stefan Roesch , Clay Harris , linux-s390@vger.kernel.org, linux-nilfs@vger.kernel.org, codalist@coda.cs.cmu.edu, cluster-devel@redhat.com, linux-cachefs@redhat.com, linux-ext4@vger.kernel.org, devel@lists.orangefs.org, linux-cifs@vger.kernel.org, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-block@vger.kernel.org, Alexander Viro , io-uring@vger.kernel.org, Jens Axboe , Christian Brauner , netdev@vger.kernel.org, samba-technical@lists.samba.org, linux-unionfs@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mtd@lists.infradead.org, bpf@vger.kernel.org, Pavel Begunkov , linux-btrfs@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net On Sun, Aug 27, 2023 at 09:28:26PM +0800, Hao Xu wrote: > From: Hao Xu > > Implement NOWAIT semantics for readdir. Return EAGAIN error to the > caller if it would block, like failing to get locks, or going to > do IO. > > Co-developed-by: Dave Chinner Not really. "Co-developed" implies equal development input between all the parties, which is not the case here - this patch is based on prototype I wrote, whilst you're doing the refining, testing and correctness work. In these cases with XFS code, we add a line in the commit message to say: "This is based on a patch originally written by Dave Chinner." > Signed-off-by: Dave Chinner > Signed-off-by: Hao Xu > [fixes deadlock issue, tweak code style] With a signoff chain like you already have. In the end you'll also get a RVB from me, which seems rather wrong to me if I've apparently been "co-developing" the code.... .... > @@ -156,7 +157,9 @@ xfs_dir2_block_getdents( > if (xfs_dir2_dataptr_to_db(geo, ctx->pos) > geo->datablk) > return 0; > > - error = xfs_dir3_block_read(args->trans, dp, &bp); > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) > + flags |= XFS_DABUF_NOWAIT; > + error = xfs_dir3_block_read(args->trans, dp, flags, &bp); > if (error) > return error; > Given we do this same check in both block and leaf formats to set XFS_DABUF_NOWAIT, and we do the DIR_CONTEXT_F_NOWAIT check in xfs_readdir() as well, we should probably do this check once at the higher level and pass flags down from there with XFS_DABUF_NOWAIT already set. > @@ -240,6 +243,7 @@ xfs_dir2_block_getdents( > STATIC int > xfs_dir2_leaf_readbuf( > struct xfs_da_args *args, > + struct dir_context *ctx, > size_t bufsize, > xfs_dir2_off_t *cur_off, > xfs_dablk_t *ra_blk, > @@ -258,10 +262,15 @@ xfs_dir2_leaf_readbuf( > struct xfs_iext_cursor icur; > int ra_want; > int error = 0; > - > - error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > - if (error) > - goto out; > + unsigned int flags = 0; > + > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { > + flags |= XFS_DABUF_NOWAIT; > + } else { > + error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > + if (error) > + goto out; > + } Especially as, in hindsight, this doesn't make a whole lot of sense. If XFS_DABUF_NOWAIT is set, we keep going until xfs_ilock_data_map_shared_nowait() where we call xfs_need_iread_extents() to see if we need to read the extents in and abort at that point. So, really, we shouldn't get this far with nowait semantics if we haven't read the extents in yet - we're supposed to already have the inode locked here and so we should have already checked this condition before we bother locking the inode... i.e. all we should be doing here is this: if (!(flags & XFS_DABUF_NOWAIT)) { error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); if (error) goto out; } And then we don't need to pass the VFS dir_context down into low level XFS functions unnecessarily. > > /* > * Look for mapped directory blocks at or above the current offset. > @@ -280,7 +289,7 @@ xfs_dir2_leaf_readbuf( > new_off = xfs_dir2_da_to_byte(geo, map.br_startoff); > if (new_off > *cur_off) > *cur_off = new_off; > - error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, 0, &bp); > + error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, flags, &bp); > if (error) > goto out; > > @@ -360,6 +369,7 @@ xfs_dir2_leaf_getdents( > int byteoff; /* offset in current block */ > unsigned int offset = 0; > int error = 0; /* error return value */ > + int written = 0; > > /* > * If the offset is at or past the largest allowed value, > @@ -391,10 +401,17 @@ xfs_dir2_leaf_getdents( > bp = NULL; > } > > - if (*lock_mode == 0) > - *lock_mode = xfs_ilock_data_map_shared(dp); > - error = xfs_dir2_leaf_readbuf(args, bufsize, &curoff, > - &rablk, &bp); > + if (*lock_mode == 0) { > + *lock_mode = > + xfs_ilock_data_map_shared_generic(dp, > + ctx->flags & DIR_CONTEXT_F_NOWAIT); > + if (!*lock_mode) { > + error = -EAGAIN; > + break; > + } > + } > + error = xfs_dir2_leaf_readbuf(args, ctx, bufsize, > + &curoff, &rablk, &bp); int xfs_ilock_readdir( struct xfs_inode *ip, int flags) { if (flags & XFS_DABUF_NOWAIT) { if (!xfs_ilock_nowait(dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(dp); } And then this code simply becomes: if (*lock_mode == 0) *lock_mode = xfs_ilock_readdir(ip, flags); > if (error || !bp) > break; > > @@ -479,6 +496,7 @@ xfs_dir2_leaf_getdents( > */ > offset += length; > curoff += length; > + written += length; > /* bufsize may have just been a guess; don't go negative */ > bufsize = bufsize > length ? bufsize - length : 0; > } > @@ -492,6 +510,8 @@ xfs_dir2_leaf_getdents( > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > if (bp) > xfs_trans_brelse(args->trans, bp); > + if (error == -EAGAIN && written > 0) > + error = 0; > return error; > } > > @@ -514,6 +534,7 @@ xfs_readdir( > unsigned int lock_mode; > bool isblock; > int error; > + bool nowait; > > trace_xfs_readdir(dp); > > @@ -531,7 +552,11 @@ xfs_readdir( > if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) > return xfs_dir2_sf_getdents(&args, ctx); > > - lock_mode = xfs_ilock_data_map_shared(dp); > + nowait = ctx->flags & DIR_CONTEXT_F_NOWAIT; > + lock_mode = xfs_ilock_data_map_shared_generic(dp, nowait); > + if (!lock_mode) > + return -EAGAIN; > + Given what I said above: if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { /* * If we need to read extents, then we must do IO * and we must use exclusive locking. We don't want * to do either of those things, so just bail if we * have to read extents. Doing this check explicitly * here means we don't have to do it anywhere else * in the XFS_DABUF_NOWAIT path. */ if (xfs_need_iread_extents(&ip->i_df)) return -EAGAIN; flags |= XFS_DABUF_NOWAIT; } lock_mode = xfs_ilock_readdir(dp, flags); And with this change, we probably should be marking the entire operation as having nowait semantics. i.e. using args->op_flags here and only use XFS_DABUF_NOWAIT for the actual IO. ie. args->op_flags |= XFS_DA_OP_NOWAIT; This makes it clear that the entire directory op should run under NOWAIT constraints, and it avoids needing to pass an extra flag through the stack. That then makes the readdir locking function look like this: /* * When we are locking an inode for readdir, we need to ensure that * the extents have been read in first. This requires the inode to * be locked exclusively across the extent read, but otherwise we * want to use shared locking. * * For XFS_DA_OP_NOWAIT operations, we do an up-front check to see * if the extents have been read in, so all we need to do in this * case is a shared try-lock as we never need exclusive locking in * this path. */ static int xfs_ilock_readdir( struct xfs_da_args *args) { if (args->op_flags & XFS_DA_OP_NOWAIT) { if (!xfs_ilock_nowait(args->dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(args->dp); } > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 9e62cc500140..d088f7d0c23a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -120,6 +120,33 @@ xfs_ilock_data_map_shared( > return lock_mode; > } > > +/* > + * Similar to xfs_ilock_data_map_shared(), except that it will only try to lock > + * the inode in shared mode if the extents are already in memory. If it fails to > + * get the lock or has to do IO to read the extent list, fail the operation by > + * returning 0 as the lock mode. > + */ > +uint > +xfs_ilock_data_map_shared_nowait( > + struct xfs_inode *ip) > +{ > + if (xfs_need_iread_extents(&ip->i_df)) > + return 0; > + if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED)) > + return 0; > + return XFS_ILOCK_SHARED; > +} > + > +int > +xfs_ilock_data_map_shared_generic( > + struct xfs_inode *dp, > + bool nowait) > +{ > + if (nowait) > + return xfs_ilock_data_map_shared_nowait(dp); > + return xfs_ilock_data_map_shared(dp); > +} And all this "generic" locking stuff goes away. FWIW, IMO, "generic" is a poor name for an XFS function as there's nothing "generic" in XFS. We tend name the functions after what they do, not some abstract concept. Leave "generic" as a keyword for widely used core infrastructure functions, not niche, one-off use cases like this. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [PATCH 02/11] xfs: add NOWAIT semantics for readdir Date: Mon, 4 Sep 2023 11:02:32 +1000 Message-ID: References: <20230827132835.1373581-1-hao.xu@linux.dev> <20230827132835.1373581-3-hao.xu@linux.dev> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1693789365; h=from:from:sender:sender:reply-to:subject:subject: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:list-id:list-help:list-unsubscribe: list-subscribe:list-post; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=IpMme351RVAzAXF1hYfi9+gH72euasEgg3PwyrofRlMvd1VU5sjsEp/dDRdRwlJEoZcRRu 3rLx2hyMAyp3WpUwKqlcXAw7nlWRJlPU5IHjCgS8XVRgf43/aSmv27C8eyAF7EX4EsQenR S93794cakUZTOh/7tPaguluif7hfT94= In-Reply-To: <20230827132835.1373581-3-hao.xu@linux.dev> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: cluster-devel-bounces@redhat.com Sender: "Cluster-devel" Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Hao Xu Cc: Wanpeng Li , "Darrick J . Wong" , Dominique Martinet , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Stefan Roesch , Clay Harris , linux-s390@vger.kernel.org, linux-nilfs@vger.kernel.org, codalist@coda.cs.cmu.edu, cluster-devel@redhat.com, linux-cachefs@redhat.com, linux-ext4@vger.kernel.org, devel@lists.orangefs.org, linux-cifs@vger.kernel.org, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-block@vger.kernel.org, Alexander Viro , io-uring@vger.kernel.org, Jens Axboe , Christian Brauner , netdev@vger.kernel.org, samba-technical@lists.samba.org, linux-unionfs@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mtd@lists.infradead.org On Sun, Aug 27, 2023 at 09:28:26PM +0800, Hao Xu wrote: > From: Hao Xu > > Implement NOWAIT semantics for readdir. Return EAGAIN error to the > caller if it would block, like failing to get locks, or going to > do IO. > > Co-developed-by: Dave Chinner Not really. "Co-developed" implies equal development input between all the parties, which is not the case here - this patch is based on prototype I wrote, whilst you're doing the refining, testing and correctness work. In these cases with XFS code, we add a line in the commit message to say: "This is based on a patch originally written by Dave Chinner." > Signed-off-by: Dave Chinner > Signed-off-by: Hao Xu > [fixes deadlock issue, tweak code style] With a signoff chain like you already have. In the end you'll also get a RVB from me, which seems rather wrong to me if I've apparently been "co-developing" the code.... .... > @@ -156,7 +157,9 @@ xfs_dir2_block_getdents( > if (xfs_dir2_dataptr_to_db(geo, ctx->pos) > geo->datablk) > return 0; > > - error = xfs_dir3_block_read(args->trans, dp, &bp); > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) > + flags |= XFS_DABUF_NOWAIT; > + error = xfs_dir3_block_read(args->trans, dp, flags, &bp); > if (error) > return error; > Given we do this same check in both block and leaf formats to set XFS_DABUF_NOWAIT, and we do the DIR_CONTEXT_F_NOWAIT check in xfs_readdir() as well, we should probably do this check once at the higher level and pass flags down from there with XFS_DABUF_NOWAIT already set. > @@ -240,6 +243,7 @@ xfs_dir2_block_getdents( > STATIC int > xfs_dir2_leaf_readbuf( > struct xfs_da_args *args, > + struct dir_context *ctx, > size_t bufsize, > xfs_dir2_off_t *cur_off, > xfs_dablk_t *ra_blk, > @@ -258,10 +262,15 @@ xfs_dir2_leaf_readbuf( > struct xfs_iext_cursor icur; > int ra_want; > int error = 0; > - > - error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > - if (error) > - goto out; > + unsigned int flags = 0; > + > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { > + flags |= XFS_DABUF_NOWAIT; > + } else { > + error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > + if (error) > + goto out; > + } Especially as, in hindsight, this doesn't make a whole lot of sense. If XFS_DABUF_NOWAIT is set, we keep going until xfs_ilock_data_map_shared_nowait() where we call xfs_need_iread_extents() to see if we need to read the extents in and abort at that point. So, really, we shouldn't get this far with nowait semantics if we haven't read the extents in yet - we're supposed to already have the inode locked here and so we should have already checked this condition before we bother locking the inode... i.e. all we should be doing here is this: if (!(flags & XFS_DABUF_NOWAIT)) { error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); if (error) goto out; } And then we don't need to pass the VFS dir_context down into low level XFS functions unnecessarily. > > /* > * Look for mapped directory blocks at or above the current offset. > @@ -280,7 +289,7 @@ xfs_dir2_leaf_readbuf( > new_off = xfs_dir2_da_to_byte(geo, map.br_startoff); > if (new_off > *cur_off) > *cur_off = new_off; > - error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, 0, &bp); > + error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, flags, &bp); > if (error) > goto out; > > @@ -360,6 +369,7 @@ xfs_dir2_leaf_getdents( > int byteoff; /* offset in current block */ > unsigned int offset = 0; > int error = 0; /* error return value */ > + int written = 0; > > /* > * If the offset is at or past the largest allowed value, > @@ -391,10 +401,17 @@ xfs_dir2_leaf_getdents( > bp = NULL; > } > > - if (*lock_mode == 0) > - *lock_mode = xfs_ilock_data_map_shared(dp); > - error = xfs_dir2_leaf_readbuf(args, bufsize, &curoff, > - &rablk, &bp); > + if (*lock_mode == 0) { > + *lock_mode = > + xfs_ilock_data_map_shared_generic(dp, > + ctx->flags & DIR_CONTEXT_F_NOWAIT); > + if (!*lock_mode) { > + error = -EAGAIN; > + break; > + } > + } > + error = xfs_dir2_leaf_readbuf(args, ctx, bufsize, > + &curoff, &rablk, &bp); int xfs_ilock_readdir( struct xfs_inode *ip, int flags) { if (flags & XFS_DABUF_NOWAIT) { if (!xfs_ilock_nowait(dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(dp); } And then this code simply becomes: if (*lock_mode == 0) *lock_mode = xfs_ilock_readdir(ip, flags); > if (error || !bp) > break; > > @@ -479,6 +496,7 @@ xfs_dir2_leaf_getdents( > */ > offset += length; > curoff += length; > + written += length; > /* bufsize may have just been a guess; don't go negative */ > bufsize = bufsize > length ? bufsize - length : 0; > } > @@ -492,6 +510,8 @@ xfs_dir2_leaf_getdents( > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > if (bp) > xfs_trans_brelse(args->trans, bp); > + if (error == -EAGAIN && written > 0) > + error = 0; > return error; > } > > @@ -514,6 +534,7 @@ xfs_readdir( > unsigned int lock_mode; > bool isblock; > int error; > + bool nowait; > > trace_xfs_readdir(dp); > > @@ -531,7 +552,11 @@ xfs_readdir( > if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) > return xfs_dir2_sf_getdents(&args, ctx); > > - lock_mode = xfs_ilock_data_map_shared(dp); > + nowait = ctx->flags & DIR_CONTEXT_F_NOWAIT; > + lock_mode = xfs_ilock_data_map_shared_generic(dp, nowait); > + if (!lock_mode) > + return -EAGAIN; > + Given what I said above: if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { /* * If we need to read extents, then we must do IO * and we must use exclusive locking. We don't want * to do either of those things, so just bail if we * have to read extents. Doing this check explicitly * here means we don't have to do it anywhere else * in the XFS_DABUF_NOWAIT path. */ if (xfs_need_iread_extents(&ip->i_df)) return -EAGAIN; flags |= XFS_DABUF_NOWAIT; } lock_mode = xfs_ilock_readdir(dp, flags); And with this change, we probably should be marking the entire operation as having nowait semantics. i.e. using args->op_flags here and only use XFS_DABUF_NOWAIT for the actual IO. ie. args->op_flags |= XFS_DA_OP_NOWAIT; This makes it clear that the entire directory op should run under NOWAIT constraints, and it avoids needing to pass an extra flag through the stack. That then makes the readdir locking function look like this: /* * When we are locking an inode for readdir, we need to ensure that * the extents have been read in first. This requires the inode to * be locked exclusively across the extent read, but otherwise we * want to use shared locking. * * For XFS_DA_OP_NOWAIT operations, we do an up-front check to see * if the extents have been read in, so all we need to do in this * case is a shared try-lock as we never need exclusive locking in * this path. */ static int xfs_ilock_readdir( struct xfs_da_args *args) { if (args->op_flags & XFS_DA_OP_NOWAIT) { if (!xfs_ilock_nowait(args->dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(args->dp); } > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 9e62cc500140..d088f7d0c23a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -120,6 +120,33 @@ xfs_ilock_data_map_shared( > return lock_mode; > } > > +/* > + * Similar to xfs_ilock_data_map_shared(), except that it will only try to lock > + * the inode in shared mode if the extents are already in memory. If it fails to > + * get the lock or has to do IO to read the extent list, fail the operation by > + * returning 0 as the lock mode. > + */ > +uint > +xfs_ilock_data_map_shared_nowait( > + struct xfs_inode *ip) > +{ > + if (xfs_need_iread_extents(&ip->i_df)) > + return 0; > + if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED)) > + return 0; > + return XFS_ILOCK_SHARED; > +} > + > +int > +xfs_ilock_data_map_shared_generic( > + struct xfs_inode *dp, > + bool nowait) > +{ > + if (nowait) > + return xfs_ilock_data_map_shared_nowait(dp); > + return xfs_ilock_data_map_shared(dp); > +} And all this "generic" locking stuff goes away. FWIW, IMO, "generic" is a poor name for an XFS function as there's nothing "generic" in XFS. We tend name the functions after what they do, not some abstract concept. Leave "generic" as a keyword for widely used core infrastructure functions, not niche, one-off use cases like this. Cheers, Dave. -- Dave Chinner david@fromorbit.com 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 14896CA0FE9 for ; Mon, 4 Sep 2023 01:03:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=VRpYh9eXmuzZ6AXoulruXY6cyWQPmZUDkt4WWlYHdJc=; b=tAQZpeCH2qq7zB N1WEWGZDbpv35Rtsm7gv8u1lWSYbjFb49ExxdtUdu5ID/gpfBfiHPDOCdNhhYHvFC0b7mot18cOwj EWgT69Aa4l3I8/3n3yEeFe9xo5/SqPg8Xp5lSotE+DDvmZsWx2RHcFGysUuJ7ftK/zKuSLtRsiKY4 7qlB+IS+UC2SdaWFo+gLitvgd0MNEVNJbRzRL8SsBQWkCbNYa565uwb+z3k3RLaYCfWnIQQo6yQKG m/EDeYhmhV7cMyRfs4i6TkrR+qkbWMHGws+zQmycpxIJ2HPnQ3k2xMOf+2lRQ3qoaz0ldYrQAcDS9 bH8nFj/g3a2wIXwEizfg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qcxzL-00363F-2Y; Mon, 04 Sep 2023 01:02:47 +0000 Received: from mail-pl1-x62d.google.com ([2607:f8b0:4864:20::62d]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qcxzI-00361O-1b for linux-mtd@lists.infradead.org; Mon, 04 Sep 2023 01:02:46 +0000 Received: by mail-pl1-x62d.google.com with SMTP id d9443c01a7336-1bf55a81eeaso2041515ad.0 for ; Sun, 03 Sep 2023 18:02:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1693789356; x=1694394156; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=nRlD9NcFNg7w+4VQaUknhe3yFzv199i50nmpuijfeVO2TkHXChMK4Qb2NyqdFogYjS VziMR9VKAPUOYM7ek71cta9G6jZ1AGt/zIK40lHdeOpJWg/us8UA09dRbj7X+A5UZ2oI +ee9rSg3m0LTeXMPA/HbK9ySp9+N/+Z5CmO4Ncof2YB/CmQQ6XzChB4rAI3Xo548qUYK RbVpFmWNgRJAxKbYRqywr9VbS/rrlQYQep6ow+Q+n1Ds9oOSVW3uir9y58FCgl10ab+1 u102R5+u59nuuKUDCeKNEycto6laha7U0DrSKR0daOhCBI/9LDevyrfqy6FHWanucL1E dz8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693789356; x=1694394156; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=7SkjlWpTOnFF4QpgAAxUR7lqv3IHtXsX9Ov+YWVrmXw=; b=bSh+3QKRsyEazw4ACnRnbUYNdKJoiCFd90hSQJH9ljVxoPpmOh23dbSwmfR5Mscp4b cMNfTZcur0Tj/WceYkn2ARn7Mn23j4uN8ebKVKpRSWiVpz40OzU26p1RWgHl0HsgJK3n 2PfJd3Nb4C+Cj+qAfSJ86ebLI7GkHMdvhgABmFu1lDx+ZhfHmLUqAejxBvJn5uc70yX+ 1h84/BT1v+vT4eOAYyBg4NkXGNyOTCWzUJ6dkRdgAIYIGWE2ye5yn113r5ClOQu0V+z4 YBapIuPxUSLuVvgfv6g4JdfA60czH+nvZ5m1/y2I1n8lZy+zA9j0AnnC9R7sAR3CQeFS 7fmw== X-Gm-Message-State: AOJu0YzT18p/Wg9ZrSJm2K4c65pu4i+iUq+EW54BOyrzj2mZNj1p61VI mkxV7VROOJW5xdec4Iq08GgDXw== X-Google-Smtp-Source: AGHT+IGGrky2P+bBuH4AKgWBeYdqXOP6u+qaOJyeKMBTbWb7BRdsdnk0jjxmGVwySX0sZe8eaRNDcw== X-Received: by 2002:a17:902:ecc8:b0:1c1:fe97:bf34 with SMTP id a8-20020a170902ecc800b001c1fe97bf34mr8040994plh.24.1693789355853; Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dread.disaster.area (pa49-195-66-88.pa.nsw.optusnet.com.au. [49.195.66.88]) by smtp.gmail.com with ESMTPSA id d4-20020a170902c18400b001bdcafcf8d3sm6351806pld.69.2023.09.03.18.02.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Sep 2023 18:02:35 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qcxz6-00AVA9-2L; Mon, 04 Sep 2023 11:02:32 +1000 Date: Mon, 4 Sep 2023 11:02:32 +1000 From: Dave Chinner To: Hao Xu Cc: io-uring@vger.kernel.org, Jens Axboe , Dominique Martinet , Pavel Begunkov , Christian Brauner , Alexander Viro , Stefan Roesch , Clay Harris , "Darrick J . Wong" , linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-cachefs@redhat.com, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-unionfs@vger.kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, linux-btrfs@vger.kernel.org, codalist@coda.cs.cmu.edu, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com, linux-mm@kvack.org, linux-nilfs@vger.kernel.org, devel@lists.orangefs.org, linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, linux-mtd@lists.infradead.org, Wanpeng Li Subject: Re: [PATCH 02/11] xfs: add NOWAIT semantics for readdir Message-ID: References: <20230827132835.1373581-1-hao.xu@linux.dev> <20230827132835.1373581-3-hao.xu@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230827132835.1373581-3-hao.xu@linux.dev> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230903_180244_755020_0F9208F8 X-CRM114-Status: GOOD ( 45.22 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org On Sun, Aug 27, 2023 at 09:28:26PM +0800, Hao Xu wrote: > From: Hao Xu > > Implement NOWAIT semantics for readdir. Return EAGAIN error to the > caller if it would block, like failing to get locks, or going to > do IO. > > Co-developed-by: Dave Chinner Not really. "Co-developed" implies equal development input between all the parties, which is not the case here - this patch is based on prototype I wrote, whilst you're doing the refining, testing and correctness work. In these cases with XFS code, we add a line in the commit message to say: "This is based on a patch originally written by Dave Chinner." > Signed-off-by: Dave Chinner > Signed-off-by: Hao Xu > [fixes deadlock issue, tweak code style] With a signoff chain like you already have. In the end you'll also get a RVB from me, which seems rather wrong to me if I've apparently been "co-developing" the code.... .... > @@ -156,7 +157,9 @@ xfs_dir2_block_getdents( > if (xfs_dir2_dataptr_to_db(geo, ctx->pos) > geo->datablk) > return 0; > > - error = xfs_dir3_block_read(args->trans, dp, &bp); > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) > + flags |= XFS_DABUF_NOWAIT; > + error = xfs_dir3_block_read(args->trans, dp, flags, &bp); > if (error) > return error; > Given we do this same check in both block and leaf formats to set XFS_DABUF_NOWAIT, and we do the DIR_CONTEXT_F_NOWAIT check in xfs_readdir() as well, we should probably do this check once at the higher level and pass flags down from there with XFS_DABUF_NOWAIT already set. > @@ -240,6 +243,7 @@ xfs_dir2_block_getdents( > STATIC int > xfs_dir2_leaf_readbuf( > struct xfs_da_args *args, > + struct dir_context *ctx, > size_t bufsize, > xfs_dir2_off_t *cur_off, > xfs_dablk_t *ra_blk, > @@ -258,10 +262,15 @@ xfs_dir2_leaf_readbuf( > struct xfs_iext_cursor icur; > int ra_want; > int error = 0; > - > - error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > - if (error) > - goto out; > + unsigned int flags = 0; > + > + if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { > + flags |= XFS_DABUF_NOWAIT; > + } else { > + error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); > + if (error) > + goto out; > + } Especially as, in hindsight, this doesn't make a whole lot of sense. If XFS_DABUF_NOWAIT is set, we keep going until xfs_ilock_data_map_shared_nowait() where we call xfs_need_iread_extents() to see if we need to read the extents in and abort at that point. So, really, we shouldn't get this far with nowait semantics if we haven't read the extents in yet - we're supposed to already have the inode locked here and so we should have already checked this condition before we bother locking the inode... i.e. all we should be doing here is this: if (!(flags & XFS_DABUF_NOWAIT)) { error = xfs_iread_extents(args->trans, dp, XFS_DATA_FORK); if (error) goto out; } And then we don't need to pass the VFS dir_context down into low level XFS functions unnecessarily. > > /* > * Look for mapped directory blocks at or above the current offset. > @@ -280,7 +289,7 @@ xfs_dir2_leaf_readbuf( > new_off = xfs_dir2_da_to_byte(geo, map.br_startoff); > if (new_off > *cur_off) > *cur_off = new_off; > - error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, 0, &bp); > + error = xfs_dir3_data_read(args->trans, dp, map.br_startoff, flags, &bp); > if (error) > goto out; > > @@ -360,6 +369,7 @@ xfs_dir2_leaf_getdents( > int byteoff; /* offset in current block */ > unsigned int offset = 0; > int error = 0; /* error return value */ > + int written = 0; > > /* > * If the offset is at or past the largest allowed value, > @@ -391,10 +401,17 @@ xfs_dir2_leaf_getdents( > bp = NULL; > } > > - if (*lock_mode == 0) > - *lock_mode = xfs_ilock_data_map_shared(dp); > - error = xfs_dir2_leaf_readbuf(args, bufsize, &curoff, > - &rablk, &bp); > + if (*lock_mode == 0) { > + *lock_mode = > + xfs_ilock_data_map_shared_generic(dp, > + ctx->flags & DIR_CONTEXT_F_NOWAIT); > + if (!*lock_mode) { > + error = -EAGAIN; > + break; > + } > + } > + error = xfs_dir2_leaf_readbuf(args, ctx, bufsize, > + &curoff, &rablk, &bp); int xfs_ilock_readdir( struct xfs_inode *ip, int flags) { if (flags & XFS_DABUF_NOWAIT) { if (!xfs_ilock_nowait(dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(dp); } And then this code simply becomes: if (*lock_mode == 0) *lock_mode = xfs_ilock_readdir(ip, flags); > if (error || !bp) > break; > > @@ -479,6 +496,7 @@ xfs_dir2_leaf_getdents( > */ > offset += length; > curoff += length; > + written += length; > /* bufsize may have just been a guess; don't go negative */ > bufsize = bufsize > length ? bufsize - length : 0; > } > @@ -492,6 +510,8 @@ xfs_dir2_leaf_getdents( > ctx->pos = xfs_dir2_byte_to_dataptr(curoff) & 0x7fffffff; > if (bp) > xfs_trans_brelse(args->trans, bp); > + if (error == -EAGAIN && written > 0) > + error = 0; > return error; > } > > @@ -514,6 +534,7 @@ xfs_readdir( > unsigned int lock_mode; > bool isblock; > int error; > + bool nowait; > > trace_xfs_readdir(dp); > > @@ -531,7 +552,11 @@ xfs_readdir( > if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) > return xfs_dir2_sf_getdents(&args, ctx); > > - lock_mode = xfs_ilock_data_map_shared(dp); > + nowait = ctx->flags & DIR_CONTEXT_F_NOWAIT; > + lock_mode = xfs_ilock_data_map_shared_generic(dp, nowait); > + if (!lock_mode) > + return -EAGAIN; > + Given what I said above: if (ctx->flags & DIR_CONTEXT_F_NOWAIT) { /* * If we need to read extents, then we must do IO * and we must use exclusive locking. We don't want * to do either of those things, so just bail if we * have to read extents. Doing this check explicitly * here means we don't have to do it anywhere else * in the XFS_DABUF_NOWAIT path. */ if (xfs_need_iread_extents(&ip->i_df)) return -EAGAIN; flags |= XFS_DABUF_NOWAIT; } lock_mode = xfs_ilock_readdir(dp, flags); And with this change, we probably should be marking the entire operation as having nowait semantics. i.e. using args->op_flags here and only use XFS_DABUF_NOWAIT for the actual IO. ie. args->op_flags |= XFS_DA_OP_NOWAIT; This makes it clear that the entire directory op should run under NOWAIT constraints, and it avoids needing to pass an extra flag through the stack. That then makes the readdir locking function look like this: /* * When we are locking an inode for readdir, we need to ensure that * the extents have been read in first. This requires the inode to * be locked exclusively across the extent read, but otherwise we * want to use shared locking. * * For XFS_DA_OP_NOWAIT operations, we do an up-front check to see * if the extents have been read in, so all we need to do in this * case is a shared try-lock as we never need exclusive locking in * this path. */ static int xfs_ilock_readdir( struct xfs_da_args *args) { if (args->op_flags & XFS_DA_OP_NOWAIT) { if (!xfs_ilock_nowait(args->dp, XFS_ILOCK_SHARED)) return -EAGAIN; return XFS_ILOCK_SHARED; } return xfs_ilock_data_map_shared(args->dp); } > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 9e62cc500140..d088f7d0c23a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -120,6 +120,33 @@ xfs_ilock_data_map_shared( > return lock_mode; > } > > +/* > + * Similar to xfs_ilock_data_map_shared(), except that it will only try to lock > + * the inode in shared mode if the extents are already in memory. If it fails to > + * get the lock or has to do IO to read the extent list, fail the operation by > + * returning 0 as the lock mode. > + */ > +uint > +xfs_ilock_data_map_shared_nowait( > + struct xfs_inode *ip) > +{ > + if (xfs_need_iread_extents(&ip->i_df)) > + return 0; > + if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED)) > + return 0; > + return XFS_ILOCK_SHARED; > +} > + > +int > +xfs_ilock_data_map_shared_generic( > + struct xfs_inode *dp, > + bool nowait) > +{ > + if (nowait) > + return xfs_ilock_data_map_shared_nowait(dp); > + return xfs_ilock_data_map_shared(dp); > +} And all this "generic" locking stuff goes away. FWIW, IMO, "generic" is a poor name for an XFS function as there's nothing "generic" in XFS. We tend name the functions after what they do, not some abstract concept. Leave "generic" as a keyword for widely used core infrastructure functions, not niche, one-off use cases like this. Cheers, Dave. -- Dave Chinner david@fromorbit.com ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/