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 5FD2BC001E0 for ; Thu, 27 Jul 2023 14:27:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233703AbjG0O1j (ORCPT ); Thu, 27 Jul 2023 10:27:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58444 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231927AbjG0O1i (ORCPT ); Thu, 27 Jul 2023 10:27:38 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 584B52D56; Thu, 27 Jul 2023 07:27:36 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id DB4DB61E97; Thu, 27 Jul 2023 14:27:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CD209C433C7; Thu, 27 Jul 2023 14:27:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1690468055; bh=wyJbJTjZXB9lB3IuTsMaPPp1mhhF4v2pSj0OspQtZjc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MCNPDnXnZ5I7x0DsnXFDcrAvWiksnUbpnFkir2T+K25//KTzc2474Qk25iQhLdkkH z5VlMZu09uPhVZ6gnLLjWgqsjQhVwrICSujk3t7qDf9iENcCfm+rr8pK9Ow1za9TNn ekdpH81na0T6l7WW1EkH8QqKKi7hnrXTd1pqwwtoavbDy5VXNfMVE5RsZ/r7uKPM92 sH0RQQUKf0fPIVJJTBKI6bjCoM5DlL11iUvBuX2WIvG27j6BzCgZVKU9TaUMimOZ5y J1FypBmY2eEIQL8O8FcMgoBqwQeQU1VipDs2ajIKA5IxzeQpVs6soZ6p+xjPrEM64m Avvl9vnEOR8Tw== Date: Thu, 27 Jul 2023 16:27:30 +0200 From: Christian Brauner To: Hao Xu , djwong@kernel.org, Dave Chinner , Jens Axboe Cc: io-uring@vger.kernel.org, Dominique Martinet , Pavel Begunkov , Alexander Viro , Stefan Roesch , Clay Harris , linux-fsdevel@vger.kernel.org, Wanpeng Li Subject: Re: [PATCH 3/5] io_uring: add support for getdents Message-ID: <20230727-salbe-kurvigen-31b410c07bb9@brauner> References: <20230718132112.461218-1-hao.xu@linux.dev> <20230718132112.461218-4-hao.xu@linux.dev> <20230726-leinen-basisarbeit-13ae322690ff@brauner> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: io-uring@vger.kernel.org On Thu, Jul 27, 2023 at 07:51:19PM +0800, Hao Xu wrote: > On 7/26/23 23:00, Christian Brauner wrote: > > On Tue, Jul 18, 2023 at 09:21:10PM +0800, Hao Xu wrote: > > > From: Hao Xu > > > > > > This add support for getdents64 to io_uring, acting exactly like the > > > syscall: the directory is iterated from it's current's position as > > > stored in the file struct, and the file's position is updated exactly as > > > if getdents64 had been called. > > > > > > For filesystems that support NOWAIT in iterate_shared(), try to use it > > > first; if a user already knows the filesystem they use do not support > > > nowait they can force async through IOSQE_ASYNC in the sqe flags, > > > avoiding the need to bounce back through a useless EAGAIN return. > > > > > > Co-developed-by: Dominique Martinet > > > Signed-off-by: Dominique Martinet > > > Signed-off-by: Hao Xu > > > --- > > > include/uapi/linux/io_uring.h | 7 +++++ > > > io_uring/fs.c | 55 +++++++++++++++++++++++++++++++++++ > > > io_uring/fs.h | 3 ++ > > > io_uring/opdef.c | 8 +++++ > > > 4 files changed, 73 insertions(+) > > > > > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > > > index 36f9c73082de..b200b2600622 100644 > > > --- a/include/uapi/linux/io_uring.h > > > +++ b/include/uapi/linux/io_uring.h > > > @@ -65,6 +65,7 @@ struct io_uring_sqe { > > > __u32 xattr_flags; > > > __u32 msg_ring_flags; > > > __u32 uring_cmd_flags; > > > + __u32 getdents_flags; > > > }; > > > __u64 user_data; /* data to be passed back at completion time */ > > > /* pack this to avoid bogus arm OABI complaints */ > > > @@ -235,6 +236,7 @@ enum io_uring_op { > > > IORING_OP_URING_CMD, > > > IORING_OP_SEND_ZC, > > > IORING_OP_SENDMSG_ZC, > > > + IORING_OP_GETDENTS, > > > /* this goes last, obviously */ > > > IORING_OP_LAST, > > > @@ -273,6 +275,11 @@ enum io_uring_op { > > > */ > > > #define SPLICE_F_FD_IN_FIXED (1U << 31) /* the last bit of __u32 */ > > > +/* > > > + * sqe->getdents_flags > > > + */ > > > +#define IORING_GETDENTS_REWIND (1U << 0) > > > + > > > /* > > > * POLL_ADD flags. Note that since sqe->poll_events is the flag space, the > > > * command flags for POLL_ADD are stored in sqe->len. > > > diff --git a/io_uring/fs.c b/io_uring/fs.c > > > index f6a69a549fd4..480f25677fed 100644 > > > --- a/io_uring/fs.c > > > +++ b/io_uring/fs.c > > > @@ -47,6 +47,13 @@ struct io_link { > > > int flags; > > > }; > > > +struct io_getdents { > > > + struct file *file; > > > + struct linux_dirent64 __user *dirent; > > > + unsigned int count; > > > + int flags; > > > +}; > > > + > > > int io_renameat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > > > { > > > struct io_rename *ren = io_kiocb_to_cmd(req, struct io_rename); > > > @@ -291,3 +298,51 @@ void io_link_cleanup(struct io_kiocb *req) > > > putname(sl->oldpath); > > > putname(sl->newpath); > > > } > > > + > > > +int io_getdents_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > > > +{ > > > + struct io_getdents *gd = io_kiocb_to_cmd(req, struct io_getdents); > > > + > > > + if (READ_ONCE(sqe->off) != 0) > > > + return -EINVAL; > > > + > > > + gd->dirent = u64_to_user_ptr(READ_ONCE(sqe->addr)); > > > + gd->count = READ_ONCE(sqe->len); > > > + > > > + return 0; > > > +} > > > + > > > +int io_getdents(struct io_kiocb *req, unsigned int issue_flags) > > > +{ > > > + struct io_getdents *gd = io_kiocb_to_cmd(req, struct io_getdents); > > > + struct file *file = req->file; > > > + unsigned long getdents_flags = 0; > > > + bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > > > > Hm, I'm not sure what exactly the rules are for IO_URING_F_NONBLOCK. > > But to point this out: > > > > vfs_getdents() > > -> iterate_dir() > > { > > if (shared) > > res = down_read_killable(&inode->i_rwsem); > > else > > res = down_write_killable(&inode->i_rwsem); > > } > > > > which means you can still end up sleeping here before you go into a > > filesystem that does actually support non-waiting getdents. So if you > > have concurrent operations that grab inode lock (touch, mkdir etc) you > > can end up sleeping here. > > > > Is that intentional or an oversight? If the former can someone please > > explain the rules and why it's fine in this case? > > I actually saw this semaphore, and there is another xfs lock in > file_accessed > --> touch_atime > --> inode_update_time > --> inode->i_op->update_time == xfs_vn_update_time > > Forgot to point them out in the cover-letter..., I didn't modify them > since I'm not very sure about if we should do so, and I saw Stefan's > patchset didn't modify them too. > > My personnal thinking is we should apply trylock logic for this > inode->i_rwsem. For xfs lock in touch_atime, we should do that since it > doesn't make sense to rollback all the stuff while we are almost at the > end of getdents because of a lock. That manoeuvres around the problem. Which I'm slightly more sensitive too as this review is a rather expensive one. Plus, it seems fixable in at least two ways: For both we need to be able to tell the filesystem that a nowait atime update is requested. Simple thing seems to me to add a S_NOWAIT flag to file_time_flags and passing that via i_op->update_time() which already has a flag argument. That would likely also help kiocb_modified(). file_accessed() -> touch_atime() -> inode_update_time() -> i_op->update_time == xfs_vn_update_time() Then we have two options afaict: (1) best-effort atime update file_accessed() already has the builtin assumption that updating atime might fail for other reasons - see the comment in there. So it is somewhat best-effort already. (2) move atime update before calling into filesystem If we want to be sure that access time is updated when a readdir request is issued through io_uring then we need to have file_accessed() give a return value and expose a new helper for io_uring or modify vfs_getdents() to do something like: vfs_getdents() { if (nowait) down_read_trylock() if (!IS_DEADDIR(inode)) { ret = file_accessed(file); if (ret == -EAGAIN) goto out_unlock; f_op->iterate_shared() } } It's not unprecedented to do update atime before the actual operation has been done afaict. That's already the case in xfs_file_write_checks() which is called before anything is written. So that seems ok. Does any of these two options work for the xfs maintainers and Jens?