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 B02B1C7618A for ; Mon, 20 Mar 2023 16:19:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ED4586B0074; Mon, 20 Mar 2023 12:19:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E843D6B0078; Mon, 20 Mar 2023 12:19:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D4BA66B007B; Mon, 20 Mar 2023 12:19:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id C475C6B0074 for ; Mon, 20 Mar 2023 12:19:41 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 86881160A93 for ; Mon, 20 Mar 2023 16:19:41 +0000 (UTC) X-FDA: 80589787362.06.B5AF908 Received: from mail-yw1-f177.google.com (mail-yw1-f177.google.com [209.85.128.177]) by imf23.hostedemail.com (Postfix) with ESMTP id A8F47140017 for ; Mon, 20 Mar 2023 16:19:39 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=bgaS7ag3; spf=pass (imf23.hostedemail.com: domain of ritesh.list@gmail.com designates 209.85.128.177 as permitted sender) smtp.mailfrom=ritesh.list@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679329179; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:in-reply-to:references:dkim-signature; bh=Xs85zXA0FLZRHe7v0i3MbFeGMJjNZb7cHoH6pL8h2Mw=; b=GAyNrTN/x/7qug2+UMjLk5XZ2mV3R2AYQiKn+uneZTMmjCjnqRiuW/jo4biOSifjdOHCb4 IaXfvdRDq1RGBGkM94UBFvK7JwNT/oSz6vkXzPw05cpek0tgmVGoEL+9KgViSEYNNGRYoy Bvk6C8085cPkH6VuOLm8a+6flTe3XHo= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=bgaS7ag3; spf=pass (imf23.hostedemail.com: domain of ritesh.list@gmail.com designates 209.85.128.177 as permitted sender) smtp.mailfrom=ritesh.list@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679329179; a=rsa-sha256; cv=none; b=mfL44+0CmeFZEhHXBtuYBHGQX9N+nmJoDjgZMec1kna670uHajuwkDSxf6kbYe29eak6VB etMxt4WMEI2n0wsb6feSd0IUjswyWFwHFSp52IUfLfMJaIM7Po+L6HN77cUn/fS4+PyxDE zKIjnMEjU/66Z/vqwav5B2kyq9mP1Cc= Received: by mail-yw1-f177.google.com with SMTP id 00721157ae682-544f7c176easo88535157b3.9 for ; Mon, 20 Mar 2023 09:19:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1679329178; h=in-reply-to:subject:cc:to:from:message-id:date:from:to:cc:subject :date:message-id:reply-to; bh=Xs85zXA0FLZRHe7v0i3MbFeGMJjNZb7cHoH6pL8h2Mw=; b=bgaS7ag3vMpR3C+s9SGnH1NteRgTqbGOBzk4Z19767mMlGZNlWvni7TxiGq2aQOKse jY63BaUp4yVNPoDTvDhzUTCEn2UsMsVkDILpbLil85PLFJA1FeiyCoAhHm+xBZrVfwRV kk8MG7nbV5Y+tjMF71l5475dKiqzMpdZqLEWfodmbjOjwhlPbDzxTRhZDuW8MvFAJy7s RuTkLJcL4SKJ1ArKLK4f/X6R2l/pMRmpRE9B/vGKJYwM/vy1Eg35SssaQEW1m127i3Pg hLPcO8K3uRM3tXEf9QSw3TOvKFKxUtcptxaBdi9fevMz/2+/dLxSC5NTyzD1/t1yVHrJ J/sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679329178; h=in-reply-to:subject:cc:to:from:message-id:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=Xs85zXA0FLZRHe7v0i3MbFeGMJjNZb7cHoH6pL8h2Mw=; b=FQYwJFhcTFwdI7CE4Kl97JhJPUxMLXWuITIBpxinGv90FHY3hRSm25vUe2oysskJ4X xFENfyf5JuqDXQWskIHlx7FmiqHB4wkzCWZMjj2Qu33OVk8ravAyp057AfIg6/tswZdG hjaZHNWOtGaqxybkMvz9DZFImextsmwv112MzPd9pz2V1gX7ds4GQsbdX0r/UzdSKOMt 3ABjEBOSiN/ovS/IcHPJSIHqBjV15yz6tNopncMQ19eGV1UVR9oaomWm5/Q9r51oSTHQ xUaCUi3vThF2fg6QfTxuZ1XwfbR8uBBlKF5qNGsLNUH1SjX/ltc+i43n8be2jftKLrnt GbMg== X-Gm-Message-State: AO0yUKWAK/9ZraC0LQ2gUOEkcLer8AB17lwHCqYptOeDUVO72wTY3h0R ieprtY3T70az6M0KO9QHcKlwvey5xX1c4Q== X-Google-Smtp-Source: AK7set/dk11fvFG53i/glaHTQimV/h1JnY1DP0NCrQOqfwxYSVLnqtWnVbHY4fVYSHjCxB5gZkCpAw== X-Received: by 2002:a17:902:f243:b0:1a1:a5d9:146d with SMTP id j3-20020a170902f24300b001a1a5d9146dmr11509080plc.65.1679328704886; Mon, 20 Mar 2023 09:11:44 -0700 (PDT) Received: from rh-tp ([2406:7400:63:469f:1474:7c59:3a57:aab6]) by smtp.gmail.com with ESMTPSA id x13-20020a1709027c0d00b001a06677948dsm6891969pll.293.2023.03.20.09.11.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Mar 2023 09:11:44 -0700 (PDT) Date: Mon, 20 Mar 2023 21:41:25 +0530 Message-Id: <87ttyfz9le.fsf@doe.com> From: Ritesh Harjani (IBM) To: "Darrick J. Wong" 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 In-Reply-To: <20230316154143.GA11351@frogsfrogsfrogs> X-Rspamd-Queue-Id: A8F47140017 X-Stat-Signature: bnmr9qcdghpq48gwddn1asf6ewengcqa X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1679329179-369419 X-HE-Meta: U2FsdGVkX1+sRTZNqKerdgfdb5oLCo5LtqpSrUgPQdcG4DEOQG0YiLYpdhSUsdYGd0wpRG1NGQp0lIAeKJynPpsT0tp7ZwhAULdH645IHmTNEDno5lPD/Pj7tbSMBFE45qvtAAvrMkHTH9INqOfDTx3oPLDqmIGSTV25WwkI+xjmsxyBxFIndt6G5B/Zx4f6tVbK7Y5QyHrvR6teUWpZbbHK+YKAVcfZ110wKR51HqBTDziWlTz6EsNDIZypcH9KONL/R9w+QqJT8AeIWlriwKMhSWin0hUqrYhhyit68GQWR8V6rQD8d1tReAoxtjnZtrkrDknNCrhp5IpgrmhUq5Qw4PYdj3s2G69Ib+eqcpY9vnbGDyF4dX/qX6CzunZ57U8qSxVDORoIlsQqAs16s4bXRdvyGCQMA3DO/XlL1CoIok4cSwqewehKl6Vk5nLkf89uXOp45DNBDtKrQrqKUQMtGzQ8HhIiN8QpSI7//x8dAGED24Y1PcDXxMhG5ko6I3N9lQrfeEpQpwcR3IY1z4vSKU5iraTUdhg7/lFXREq/vzw4Fc03nZjno34Wm0SxG6klpk0dy5P5kxAZy5MwAbVuzgrKDP2cvsh+AMkT6ne97nBrbiBayQ57boHRQhhwcfpLcbHUNDfkxzKuH2jZRwvNV1AnBYy0yx+LihLQW3IfPurV2YwH1DNJHqdVndNBGlz97M/H0WhrmHKQTFr3d6hKBo4hzU6LrHWQKYSL8hidpepfYBtzEk8mtlfWFF0lMRMfhq70Tz78GgmQh/DkOGgko8VNWLhjibWQ2NMA6iE3+nvQv/hUfl5U5Lgbb30EFzr8967wMqbBcCKEargeVFVtQHf7QcnlaDyMXuthi6mQEfdwy6c+NG1wKUxP2R1a0WC5arq456HCYM/aUKeUqjyK9MXnMo158vCL6BT9o+cEefuM3Lb0Zkxw6oEExEUCBY6Thy2PrX1xEa7osQD QXuWk9Xw q898GrZcd0pDjh5u3LmNTUbl5wPFKJmGicc/NQRIOWBNzzJSgsuDAhEzWIXTjug3zpynNLXBDtEOpUQ1iYwQSZMLp/AWobhDAFJungU9T7U+/M1zqDCbkCFT3Hd+IlFOupWHurYLtZHJ7FrY0buMIF6K1nIyVu0sr6W1f3Ydjm+AS99rHFUmSmErE9ORh+x0xPExYEmlMQDc2eJIBxnXgRhgUgmh1d+iWgNuuu1xTzU4Jh8RzfouUdrC/RxnJl9bOxMoQvGwiFwp5PiIXFryxFVL3yxixuI/MVkBvIMrxknFOUrtiDPU+PMliGa/qzDgFR7ieAs2jOT0pEQs8BmClMbEqrp+aWIkwZeOnUHXoNw+7JPkpiog9bwlnR++5C5iYhSrcaPz49PA/RfzADwn3XZ1Z5027EcDAgblbNfdMmNlVYB5/FUzlDzrweSwvRPyO0CQhbo1PK2XgGCndqHGhlpYvZihDhiy2xV1T 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: "Darrick J. Wong" writes: > On Thu, Mar 16, 2023 at 08:10:29PM +0530, 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; >> + >> + 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; > > kiocb_modified() instead of calling file_remove_privs? Yes, looks likle it is a replacement for file_remove_privs and file_update_time(). >> + >> + /* >> + * 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; >> + >> + /* use IOMAP_DIO_FORCE_WAIT for unaligned of extending writes */ >> + 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; >> + >> + if (ret < 0 && ret != -EIOCBQUEUED) >> + ext2_write_failed(inode->i_mapping, offset + count); >> + >> + /* handle case for partial write or fallback to buffered write */ >> + if (ret >= 0 && iov_iter_count(from)) { >> + loff_t pos, endbyte; >> + ssize_t status; >> + ssize_t ret2; >> + >> + pos = iocb->ki_pos; >> + status = generic_perform_write(iocb, from); >> + if (unlikely(status < 0)) { >> + ret = status; >> + goto out_unlock; >> + } >> + endbyte = pos + status - 1; >> + ret2 = filemap_write_and_wait_range(inode->i_mapping, pos, >> + endbyte); >> + if (ret2 == 0) { >> + iocb->ki_pos = endbyte + 1; >> + ret += status; >> + invalidate_mapping_pages(inode->i_mapping, >> + pos >> PAGE_SHIFT, >> + endbyte >> PAGE_SHIFT); >> + } >> + } > > (Why not fall back to the actual buffered write path?) > Because then we can handle everything related to DIO in ext4_dio_file_write() itself e.g. As per the semantics of DIO we should ensure that page-cache pages are written to disk and invalidated before returning (filemap_write_and_wait_range() and invalidate_mapping_pages()) > Otherwise this looks like a reasonable first start. Thanks! -ritesh