Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 13:20:06 +0200
From:   Jan Kara <jack@suse.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        Jan Kara <jack@suse.cz>, Christoph Hellwig <hch@infradead.org>,
        "Darrick J . Wong" <djwong@kernel.org>,
        Ojaswin Mujoo <ojaswin@linux.ibm.com>,
        Disha Goel <disgoel@linux.ibm.com>,
        Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCHv5 5/9] ext2: Move direct-io to use iomap
Message-ID: <20230417112006.3bzzitsxy67jpviq@quack3>
References: <cover.1681639164.git.ritesh.list@gmail.com>
 <20553b1a61760dc6a7451e3b4fc9ba76653e6eb0.1681639164.git.ritesh.list@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20553b1a61760dc6a7451e3b4fc9ba76653e6eb0.1681639164.git.ritesh.list@gmail.com>
Precedence: bulk

On Sun 16-04-23 15:38:40, Ritesh Harjani (IBM) wrote:
> This patch converts ext2 direct-io path to iomap interface.
> - This also takes care of DIO_SKIP_HOLES part in which we return -ENOTBLK
>   from ext2_iomap_begin(), in case if the write is done on a hole.
> - This fallbacks to buffered-io in case of DIO_SKIP_HOLES or in case of
>   a partial write or if any error is detected in ext2_iomap_end().
>   We try to return -ENOTBLK in such cases.
> - For any unaligned or extending DIO writes, we pass
>   IOMAP_DIO_FORCE_WAIT flag to ensure synchronous writes.
> - For extending writes we set IOMAP_F_DIRTY in ext2_iomap_begin because
>   otherwise with dsync writes on devices that support FUA, generic_write_sync
>   won't be called and we might miss inode metadata updates.
> - Since ext2 already now uses _nolock vartiant of sync write. Hence
>   there is no inode lock problem with iomap in this patch.
> - ext2_iomap_ops are now being shared by DIO, DAX & fiemap path
> 
> Tested-by: Disha Goel <disgoel@linux.ibm.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

One comment below:

> @@ -844,6 +868,13 @@ static int
>  ext2_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>  		ssize_t written, unsigned flags, struct iomap *iomap)
>  {
> +	/*
> +	 * Switch to buffered-io in case of any error.
> +	 * Blocks allocated can be used by the buffered-io path.
> +	 */
> +	if ((flags & IOMAP_DIRECT) && (flags & IOMAP_WRITE) && written == 0)
> +		return -ENOTBLK;
> +
>  	if (iomap->type == IOMAP_MAPPED &&
>  	    written < length &&
>  	    (flags & IOMAP_WRITE))

Is this really needed? What for?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR