Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Jul 2021 16:59:00 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc:     linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
        linux-kernel@vger.kernel.org, pali@kernel.org, dsterba@suse.cz,
        aaptel@suse.com, rdunlap@infradead.org, joe@perches.com,
        mark@harmstone.com, nborisov@suse.com,
        linux-ntfs-dev@lists.sourceforge.net, anton@tuxera.com,
        dan.carpenter@oracle.com, hch@lst.de, ebiggers@kernel.org,
        andy.lavr@gmail.com, kari.argillander@gmail.com,
        oleksandr@natalenko.name
Subject: Re: [PATCH v27 02/10] fs/ntfs3: Add initialization of super block
Message-ID: <YQLQRGyKGdL00sQ7@casper.infradead.org>
References: <20210729134943.778917-1-almaz.alexandrovich@paragon-software.com>
 <20210729134943.778917-3-almaz.alexandrovich@paragon-software.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210729134943.778917-3-almaz.alexandrovich@paragon-software.com>
Precedence: bulk

On Thu, Jul 29, 2021 at 04:49:35PM +0300, Konstantin Komarov wrote:
> +static void ntfs_readahead(struct readahead_control *rac)
> +{
> +	struct address_space *mapping = rac->mapping;
> +	struct inode *inode = mapping->host;
> +	struct ntfs_inode *ni = ntfs_i(inode);
> +	u64 valid;
> +	loff_t pos;
> +
> +	if (is_resident(ni)) {
> +		/* no readahead for resident */
> +		return;
> +	}
> +
> +	if (is_compressed(ni)) {
> +		/* no readahead for compressed */
> +		return;

I'm sure this works, but it could perform better ;-)

The ->readpage path that you fall back to is synchronous (unless I
misunderstand something).  We now have readahead_expand() which lets
you ensure that there are pages in the page cache for the entire
range of the compressed extent.  So if you can create an asynchronous
decompression path (probably need your own custom bi_end_io), you can
start reads here and only unlock the pages after you've done the
decompression.

This should not gate your code being accepted upstream.  What I'd
really like to see is your buffered i/o path being converted from
buffer_heads to iomap, and iomap to gain the ability to handle
compressed extents.

> +	valid = ni->i_valid;
> +	pos = readahead_pos(rac);
> +
> +	if (valid < i_size_read(inode) && pos <= valid &&
> +	    valid < pos + readahead_length(rac)) {
> +		/* range cross 'valid'. read it page by page */
> +		return;

This, I do not understand.  Why can't ntfs_get_block / mpage_readahead
cope with a readahead that crosses i_valid?  AIUI, i_valid is basically
the same as i_size, and mpage_readahead() handles crossing i_size
just fine.