Re: [PATCH] ceph: do not fill fscache for RWF_DONTCACHE writeback

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Fri, 2026-04-03 at 08:52 +0200, Max Kellermann wrote:
> On Thu, Apr 2, 2026 at 9:44 PM Viacheslav Dubeyko <vdubeyko@redhat.com> wrote:
> > The Ceph write_begin path calls netfs_write_begin() but does not pass
> > IOCB_DONTCACHE through to trigger __folio_set_dropbehind. So,
> > folio_test_dropbehind() would never be true on the Ceph write path right now.
> > Does it make sense?
> 
> Yes, see:
> 
> > > ---
> > > Note: this is an additional feature on top of my Ceph-DONTCACHE patch,
> > > see https://lore.kernel.org/ceph-devel/20260401053109.1861724-1-max.kellermann@ionos.com/
> 
> The code in this patch is not reachable until my RWF_DONTCACHE patch
> is merged as well.
> 
> > Are you sure that caching should be always true? All other calls checks that
> > ceph_is_cache_enabled():
> > 
> > bool caching = ceph_is_cache_enabled(inode);
> 
> This function is only called if caching is enabled.

I think that such interface will be more clean and safe:

static inline void ceph_flush_fscache_write(struct inode *inode, u64 off, u64
*len, bool caching)

> 
> > 
> > > +     *len = 0;
> > > +}
> > 
> > 
> > The ceph_folio_is_cacheable() and ceph_flush_fscache_write() are out of
> > CONFIG_CEPH_FSCACHE. It doesn't look right.
> 
> All of the old code is out of CONFIG_CEPH_FSCACHE, too. Does the old
> code look right?

As far as I can see, all fscache code is under CONFIG_CEPH_FSCACHE compilation
option. If we have some issues with old code, then it makes sense to fix it. But
this code is fscache related and it should be under CONFIG_CEPH_FSCACHE
protection, from my point of view. Moreover, other fscache related code is under
CONFIG_CEPH_FSCACHE protection pretty above the code of these functions.

> 
> > > @@ -1412,11 +1427,14 @@ int ceph_submit_write(struct address_space *mapping,
> > >       bool caching = ceph_is_cache_enabled(inode);
> > >       u64 offset;
> > >       u64 len;
> > > +     u64 cache_offset, cache_len;
> > 
> > Why do you need to introduce the cache_offset and cache_len? We already have
> > offset and len.
> 
> These keep track of the region that should be submitted to fscache.
> Folios without "dropbehind" need to be excluded from that.
> 
> > >  new_request:
> > >       offset = ceph_fscrypt_page_offset(ceph_wbc->pages[0]);
> > >       len = ceph_wbc->wsize;
> > > +     cache_offset = 0;
> > 
> > Is it correct initialization? Frankly speaking, I don't quite follow why we need
> > such initialization.
> 
> Technically, cache_offset does not need to be initialized as long as
> cache_len is zero because then its value is never used. Would you feel
> more comfortable if I remove the unnecessary initializer? I wasn't
> sure which approach would raise fewer eyebrows.

I am simply trying to follow why we need in cache_offset. We are using the
offset currently:

/* Kick off an fscache write with what we have so far. */
ceph_fscache_write_to_cache(inode, offset, len, caching);


Why the offset is not good enough?

Thanks,
Slava.