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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A7E65C7618A for ; Mon, 20 Mar 2023 17:51:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EBE636B0074; Mon, 20 Mar 2023 13:51:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E46A76B0078; Mon, 20 Mar 2023 13:51:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CC1326B007B; Mon, 20 Mar 2023 13:51:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id BBC7C6B0074 for ; Mon, 20 Mar 2023 13:51:43 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 94BFB1C66B6 for ; Mon, 20 Mar 2023 17:51:43 +0000 (UTC) X-FDA: 80590019286.03.4B6E238 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by imf21.hostedemail.com (Postfix) with ESMTP id 844051C0018 for ; Mon, 20 Mar 2023 17:51:41 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=XNB8njeS; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=nuUaMpwz; spf=pass (imf21.hostedemail.com: domain of jack@suse.cz designates 195.135.220.28 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679334701; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GRnFPbzooMWZJFxO7VzdDsDtC5lwueBazWgNaHtvilc=; b=u15Ci/PSf9w08QdT7FydFv1GTMRZEcDQRl1ezm8eTZDhgSLl9vDsSn2QxsANK4OTlLHaIh MDe6uS/5eqLIHWVF/DeuOEIZ9oIPCNTLLCV82f8wj3Do7xTZpvz+bz0O9hJP0H+XGA9431 pT72cYZPCyvroeNMcdIWTuqBfBU2Dto= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=XNB8njeS; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=nuUaMpwz; spf=pass (imf21.hostedemail.com: domain of jack@suse.cz designates 195.135.220.28 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679334701; a=rsa-sha256; cv=none; b=Vo6ofZbYzpRN6goLjgr2Kat0OLgjYNOAMuIJJWIFTa0GSa1/GpfPST5hSVSMzRhSy+SyrO +FQeMDdedxmwcobBzVE4W9Nc1WZezRyLA11DQ8WxuVicp67diCDAzT+VhXNh720dxQEd9N 7eARCZAlByn4UDDIaGjzaLoaC8j1eVY= 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 E54AB21A9A; Mon, 20 Mar 2023 17:51:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1679334699; 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=GRnFPbzooMWZJFxO7VzdDsDtC5lwueBazWgNaHtvilc=; b=XNB8njeSyMhpodw27SNnoCK5OPFhkg+Hkzk0iiXFcyz0Fu0a+Ky4BTqRB9oai1AeuVID2A RPGhIyNFmZzvWLaohevzVWSvro2yUDMcm9ZDBooiWEWQPAAYS9gA31LEXdIHEanhjLcTUn mAYciBT5GyKaDvE/PWQVainu5i8BmXE= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1679334699; 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=GRnFPbzooMWZJFxO7VzdDsDtC5lwueBazWgNaHtvilc=; b=nuUaMpwzFMjPmV6fMAF8lsVvj4L2ty9+IIluQC8+fVAqUU62rmnE/1uT4B5r5K2Y2IWBm7 GolOFgJQDv9g4OAQ== 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 CF2F013416; Mon, 20 Mar 2023 17:51:39 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id a3J+MiudGGRNGAAAMHmgww (envelope-from ); Mon, 20 Mar 2023 17:51:39 +0000 Received: by quack3.suse.cz (Postfix, from userid 1000) id 206D6A0719; Mon, 20 Mar 2023 18:51:39 +0100 (CET) Date: Mon, 20 Mar 2023 18:51:39 +0100 From: Jan Kara To: "Ritesh Harjani (IBM)" Cc: Jan Kara , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, lsf-pc@lists.linux-foundation.org Subject: Re: [RFCv1][WIP] ext2: Move direct-io to use iomap Message-ID: <20230320175139.l5oqbwuae4schgcu@quack3> References: <87ttz889ns.fsf@doe.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Stat-Signature: 64tbjitmkzieu5zx4qhnqn7hgygj69kb X-Rspam-User: X-Rspamd-Queue-Id: 844051C0018 X-Rspamd-Server: rspam06 X-HE-Tag: 1679334701-269479 X-HE-Meta: U2FsdGVkX1+kxgnqK7gb92xxyEuANPhcpC3/vss8AEhgK09gP7OJ9jX/PVetDtH+1kmKqUXV0eIBrCw1OfY7o0oDT9kSGzDkNLOQE78+2CITOBO39eV0TQRXJyibJ4xOII0iKxPHxOwIWIxJiRb0JhmVnBkpdPRsAjhWeed/RhHKnuFmMB19vwnJCESE1GcA7K3danHoRCY7WFsuEuGPKKqERLqMCC+4teUAEE7R6BdHnNeSUjTzZUkmYE5t3KJ5XdSk0lXnMwMY9xvP7FEluPZaN//viGiqZSNdJflKIXQ8gW3gSwUuZX/fFXMZvRxYWD1KpAejhxwVOZ/ppjVcMxCTySzoCme4ugjwyWXoBbN+8XGKNqp9fYvGvNuxAx8wbKvuqMRyUZuaOHBHjsiJny67kpUUkLOtnPFZWWikxZh4IqSXHuTwkpD0u2UIAl1nznewenpk+nXI0cj74fa/3mOkjTIXJl/v9pxs7VPRpGeqjmi+seXm2ar/C3PJedCB6nBXKIpVoIK0BYgNclyNkxnqDbX9vWPQmEa2Fq5WGixNk9WXGdFc3BRiIzp9LkW2BK1YLYbgz6OuV670rR0LxiPkmO2rpUo1NcYW1JkCa2QQQFDOAvelbI32XZtJiOfnaRMEwE8D7Uh0CzKiMQJb0AJXjOUCbYkqTTLY6gmtpL+XwbBy98ykOZgsgyNWMSeL83B2IHbPeg0fB1BJnqaLTDFaLyCZRvhCKCk6yw5aAHmDDcWH6qxbjyZ69XfKyx6vnp/tJN75j3Px+TCx7h3uM+qqSY67Lq0nGvFbhqR8vqJ+KqswSzcgekuKvfUm3RQTZTgDneuh+f7ZH7hiu1Tu0DH6KoPHffz5Ye0DpoxZuBbGvpvDYPJCJFeLAPO193Cy3+SlwT3g4Ceim8Wu4maABj/47ewrMThLBapE9QOFOIILI6KEhoyhJsBfdhkYqNCFyh1BnoQn3Pq0z7ngbFk NP3P/NHY Q47d+suxHeLZVCkuoPQd4EOULJyTET/TYsPJPJ4sSSc9apJhjggiMmgl89DVP67pjVkm4r0K4rv0sXdm2ltc7sdvVg198Px1opOnt3xYiIqwy6CbKYRnhUalBg5RrDNazesQaUPPSj80TIADPpoj9aNbMSQDq4AU7TOuaXWhet75PlRRB+aZ2uRJpZQ/uqdcbqzd+nMEBdCshGorjyc3PSIszWVIgZyGrAqN/aB66OoDDJewfrqdeO0afVWvIWGLDBJbSHtrlbLJYToGjfWbaM9DTA1dOA/aeVyfXrkPnFJiIr9ZubgzdE9QH17ziZFkLxtA5SBRhK6gDaRWIS3IyiiufiuhX2ssJDY2wF3S0uzK6tGE= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu 16-03-23 20:10:29, Ritesh Harjani (IBM) wrote: > [DO NOT MERGE] [WORK-IN-PROGRESS] > > Hello Jan, > > This is an initial version of the patch set which I wanted to share > before today's call. This is still work in progress but atleast passes > the set of test cases which I had kept for dio testing (except 1 from my > list). > > Looks like there won't be much/any changes required from iomap side to > support ext2 moving to iomap apis. > > I will be doing some more testing specifically test generic/083 which is > occassionally failing in my testing. > Also once this is stabilized, I can do some performance testing too if you > feel so. Last I remembered we saw some performance regressions when ext4 > moved to iomap for dio. > > PS: Please ignore if there are some silly mistakes. As I said, I wanted > to get this out before today's discussion. :) > > Thanks for your help!! > > Signed-off-by: Ritesh Harjani (IBM) > --- > fs/ext2/ext2.h | 1 + > fs/ext2/file.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++ > fs/ext2/inode.c | 20 +-------- > 3 files changed, 117 insertions(+), 18 deletions(-) > > diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h > index cb78d7dcfb95..cb5e309fe040 100644 > --- a/fs/ext2/ext2.h > +++ b/fs/ext2/ext2.h > @@ -753,6 +753,7 @@ extern unsigned long ext2_count_free (struct buffer_head *, unsigned); > extern struct inode *ext2_iget (struct super_block *, unsigned long); > extern int ext2_write_inode (struct inode *, struct writeback_control *); > extern void ext2_evict_inode(struct inode *); > +extern void ext2_write_failed(struct address_space *mapping, loff_t to); > extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int); > extern int ext2_setattr (struct mnt_idmap *, struct dentry *, struct iattr *); > extern int ext2_getattr (struct mnt_idmap *, const struct path *, > diff --git a/fs/ext2/file.c b/fs/ext2/file.c > index 6b4bebe982ca..7a8561304559 100644 > --- a/fs/ext2/file.c > +++ b/fs/ext2/file.c > @@ -161,12 +161,123 @@ int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync) > return ret; > } > > +static ssize_t ext2_dio_read_iter(struct kiocb *iocb, struct iov_iter *to) > +{ > + struct file *file = iocb->ki_filp; > + struct inode *inode = file->f_mapping->host; > + ssize_t ret; > + > + inode_lock_shared(inode); > + ret = iomap_dio_rw(iocb, to, &ext2_iomap_ops, NULL, 0, NULL, 0); > + inode_unlock_shared(inode); > + > + return ret; > +} > + > +static int ext2_dio_write_end_io(struct kiocb *iocb, ssize_t size, > + int error, unsigned int flags) > +{ > + loff_t pos = iocb->ki_pos; > + struct inode *inode = file_inode(iocb->ki_filp); > + > + if (error) > + return error; > + I guess you should carry over here relevant bits of the comment from ext4_dio_write_end_io() explaining that doing i_size update here is necessary and actually safe. > + pos += size; > + if (pos > i_size_read(inode)) > + i_size_write(inode, pos); > + > + return 0; > +} > + > +static const struct iomap_dio_ops ext2_dio_write_ops = { > + .end_io = ext2_dio_write_end_io, > +}; > + > +static ssize_t ext2_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) > +{ > + struct file *file = iocb->ki_filp; > + struct inode *inode = file->f_mapping->host; > + ssize_t ret; > + unsigned int flags; > + unsigned long blocksize = inode->i_sb->s_blocksize; > + loff_t offset = iocb->ki_pos; > + loff_t count = iov_iter_count(from); > + > + > + inode_lock(inode); > + ret = generic_write_checks(iocb, from); > + if (ret <= 0) > + goto out_unlock; > + ret = file_remove_privs(file); > + if (ret) > + goto out_unlock; > + ret = file_update_time(file); > + if (ret) > + goto out_unlock; > + > + /* > + * We pass IOMAP_DIO_NOSYNC because otherwise iomap_dio_rw() > + * calls for generic_write_sync in iomap_dio_complete(). > + * Since ext2_fsync nmust be called w/o inode lock, > + * hence we pass IOMAP_DIO_NOSYNC and handle generic_write_sync() > + * ourselves. > + */ > + flags = IOMAP_DIO_NOSYNC; Meh, this is kind of ugly and we should come up with something better for simple filesystems so that they don't have to play these games. Frankly, these days I doubt there's anybody really needing inode_lock in __generic_file_fsync(). Neither sync_mapping_buffers() nor sync_inode_metadata() need inode_lock for their self-consistency. So it is only about flushing more consistent set of metadata to disk when fsync(2) races with other write(2)s to the same file so after a crash we have higher chances of seeing some real state of the file. But I'm not sure it's really worth keeping for filesystems that are still using sync_mapping_buffers(). People that care about consistency after a crash have IMHO moved to other filesystems long ago. > + > + /* use IOMAP_DIO_FORCE_WAIT for unaligned of extending writes */ ^^ or > + if (iocb->ki_pos + iov_iter_count(from) > i_size_read(inode) || > + (!IS_ALIGNED(iocb->ki_pos | iov_iter_alignment(from), blocksize))) > + flags |= IOMAP_DIO_FORCE_WAIT; > + > + ret = iomap_dio_rw(iocb, from, &ext2_iomap_ops, &ext2_dio_write_ops, > + flags, NULL, 0); > + > + if (ret == -ENOTBLK) > + ret = 0; So iomap_dio_rw() doesn't have the DIO_SKIP_HOLES behavior of blockdev_direct_IO(). Thus you have to implement that in your ext2_iomap_ops, in particular in iomap_begin... Honza -- Jan Kara SUSE Labs, CR