Received-SPF: pass (google.com: domain of linux-ext4+bounces-249-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 30 Nov 2023 15:08:59 +0100
From: Jan Kara <jack@suse.cz>
To: Ritesh Harjani <ritesh.list@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Christoph Hellwig <hch@infradead.org>,
	linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [RFC 2/3] ext2: Convert ext2 regular file buffered I/O to use
 iomap
Message-ID: <20231130140859.hdgvf24ystz2ghdv@quack3>
References: <20231130101845.mt3hhwbbpnhroefg@quack3>
 <87fs0nik0g.fsf@doe.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87fs0nik0g.fsf@doe.com>

On Thu 30-11-23 16:29:59, Ritesh Harjani wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > On Thu 30-11-23 13:15:58, Ritesh Harjani wrote:
> >> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
> >> 
> >> > Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
> >> >
> >> >> Christoph Hellwig <hch@infradead.org> writes:
> >> >>
> >> >>> On Wed, Nov 22, 2023 at 01:29:46PM +0100, Jan Kara wrote:
> >> >>>> writeback bit set. XFS plays the revalidation sequence counter games
> >> >>>> because of this so we'd have to do something similar for ext2. Not that I'd
> >> >>>> care as much about ext2 writeback performance but it should not be that
> >> >>>> hard and we'll definitely need some similar solution for ext4 anyway. Can
> >> >>>> you give that a try (as a followup "performance improvement" patch).
> >> 
> >> ok. So I am re-thinknig over this on why will a filesystem like ext2
> >> would require sequence counter check. We don't have collapse range
> >> or COW sort of operations, it is only the truncate which can race,
> >> but that should be taken care by folio_lock. And even if the partial
> >> truncate happens on a folio, since the logical to physical block mapping
> >> never changes, it should not matter if the writeback wrote data to a
> >> cached entry, right?
> >
> > Yes, so this is what I think I've already mentioned. As long as we map just
> > the block at the current offset (or a block under currently locked folio),
> > we are fine and we don't need any kind of sequence counter. But as soon as
> > we start caching any kind of mapping in iomap_writepage_ctx we need a way
> > to protect from races with truncate. So something like the sequence counter.
> >
> 
> Why do we need to protect from the race with truncate, is my question here.
> So, IMO, truncate will truncate the folio cache first before releasing the FS
> blocks. Truncation of the folio cache and the writeback path are
> protected using folio_lock()
> Truncate will clear the dirty flag of the folio before
> releasing the folio_lock() right, so writeback will not even proceed for
> folios which are not marked dirty (even if we have a cached wpc entry for
> which folio is released from folio cache).
> 
> Now coming to the stale cached wpc entry for which truncate is doing a
> partial truncation. Say, truncate ended up calling
> truncate_inode_partial_folio(). Now for such folio (it remains dirty
> after partial truncation) (for which there is a stale cached wpc entry),
> when writeback writes to the underlying stale block, there is no harm
> with that right?
> 
> Also this will "only" happen for folio which was partially truncated.
> So why do we need to have sequence counter for protecting against this
> race is my question. 

That's a very good question and it took me a while to formulate my "this
sounds problematic" feeling into a particular example :) We can still have
a race like:

write_cache_pages()
  cache extent covering 0..1MB range
  write page at offset 0k
					truncate(file, 4k)
					  drops all relevant pages
					  frees fs blocks
					pwrite(file, 4k, 4k)
					  creates dirty page in the page cache
  writes page at offset 4k to a stale block

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