linux-ext4 - Re: [RFC 2/3] ext2: Convert ext2 regular file buffered I/O to use iomap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZVw1xxNYQuHimSmx@infradead.org>
Date: Mon, 20 Nov 2023 20:44:55 -0800
From: Christoph Hellwig <hch@...radead.org>
To: "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
Cc: linux-ext4@...r.kernel.org, Jan Kara <jack@...e.cz>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [RFC 2/3] ext2: Convert ext2 regular file buffered I/O to use
 iomap

On Tue, Nov 21, 2023 at 12:35:20AM +0530, Ritesh Harjani (IBM) wrote:
> - mmap path of ext2 continues to use generic_file_vm_ops

So this means there are not space reservations taken at write fault
time.  While iomap was written with the assumption those exist, I can't
instantly spot anything that relies on them - you are just a lot more
likely to hit an -ENOSPC from ->map_blocks now.  Maybe we should add
an xfstests that covers the case where we use up more then the existing
free space through writable mmaps to ensure it fully works (where works
means at least as good as the old behavior)?

> +static ssize_t ext2_buffered_write_iter(struct kiocb *iocb,
> +					struct iov_iter *from)
> +{
> +	ssize_t ret = 0;
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +
> +	inode_lock(inode);
> +	ret = generic_write_checks(iocb, from);
> +	if (ret > 0)
> +		ret = iomap_file_buffered_write(iocb, from, &ext2_iomap_ops);
> +	inode_unlock(inode);
> +	if (ret > 0)
> +		ret = generic_write_sync(iocb, ret);
> +	return ret;
> +}

Not for now, but if we end up doing a bunch of conversation of trivial
file systems, it might be worth to eventually add a wrapper for the
above code with just the iomap_ops passed in.  Only once we see a few
duplicates, though.

> +static int ext2_write_map_blocks(struct iomap_writepage_ctx *wpc,
> +				 struct inode *inode, loff_t offset)
> +{
> +	if (offset >= wpc->iomap.offset &&
> +	    offset < wpc->iomap.offset + wpc->iomap.length)
> +		return 0;
> +
> +	return ext2_iomap_begin(inode, offset, inode->i_sb->s_blocksize,
> +				IOMAP_WRITE, &wpc->iomap, NULL);
> +}

Looking at gfs2 this also might become a pattern.  But I'm fine with
waiting for now.

> @@ -1372,7 +1428,7 @@ void ext2_set_file_ops(struct inode *inode)
>  	if (IS_DAX(inode))
>  		inode->i_mapping->a_ops = &ext2_dax_aops;
>  	else
> -		inode->i_mapping->a_ops = &ext2_aops;
> +		inode->i_mapping->a_ops = &ext2_file_aops;
>  }

Did yo run into issues in using the iomap based aops for the other uses
of ext2_aops, or are just trying to address the users one at a time?