Re: [PATCH 03/13] orangefs: Check PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES inode state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] orangefs: stale i_size overwrite during writeback in
  orangefs_inode_getattr()
--

commit 627feba60e819dc443191dbfcd63f583f8da0f24
Author: Jan Kara <jack@suse.cz>

orangefs: Check PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES inode state

This commit replaces checks of the I_DIRTY_PAGES inode state flag in
orangefs_inode_getattr() with mapping_tagged(..., PAGECACHE_TAG_DIRTY).

> diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c
> index 9ab1119ebd28a..4c4036f470cdd 100644
> --- a/fs/orangefs/orangefs-utils.c
> +++ b/fs/orangefs/orangefs-utils.c
> @@ -247,7 +247,8 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
>  	spin_lock(&inode->i_lock);
>  	/* Must have all the attributes in the mask and be within cache time. */
>  	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
> -	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
> +	    orangefs_inode->attr_valid ||
> +	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
>  		if (orangefs_inode->attr_valid) {
>  			spin_unlock(&inode->i_lock);
>  			write_inode_now(inode, 1);
> @@ -281,13 +282,14 @@ int orangefs_inode_getattr(struct inode *inode, int flags)
>  	spin_lock(&inode->i_lock);
>  	/* Must have all the attributes in the mask and be within cache time. */
>  	if ((!flags && time_before(jiffies, orangefs_inode->getattr_time)) ||
> -	    orangefs_inode->attr_valid || inode_state_read(inode) & I_DIRTY_PAGES) {
> +	    orangefs_inode->attr_valid ||
> +	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
>  		if (orangefs_inode->attr_valid) {
>  			spin_unlock(&inode->i_lock);
>  			write_inode_now(inode, 1);
>  			goto again2;
>  		}
> -		if (inode_state_read(inode) & I_DIRTY_PAGES) {
> +		if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY)) {
>  			ret = 0;
>  			goto out_unlock;
>  		}

[Severity: High]
Can orangefs_inode_getattr() overwrite inode->i_size with a stale size from
the server while pagecache writeback is in flight?

When a buffered write extends a file in orangefs_write_end(), inode->i_size
is updated in memory and the folio is marked dirty, while the server only
learns the new size once the writeback RPC in wait_for_direct_io()
completes.  Previously, I_DIRTY_PAGES stayed set in
__writeback_single_inode() across the entire do_writepages() call.
However, folio_start_writeback() clears PAGECACHE_TAG_DIRTY and sets
PAGECACHE_TAG_WRITEBACK before wait_for_direct_io() sends the write to the
server:

CPU0                                  CPU1
orangefs_writepage_locked()
  folio_start_writeback(folio)
    /* clears PAGECACHE_TAG_DIRTY,
       sets PAGECACHE_TAG_WRITEBACK */
                                      orangefs_getattr()
                                        orangefs_inode_getattr(...,
                                            ORANGEFS_GETATTR_SIZE)
                                          /* PAGECACHE_TAG_DIRTY is false */
                                          service_operation(GETATTR)
                                          inode->i_size = ...; <-- stale size
  len = i_size_read(inode);
  wait_for_direct_io(ORANGEFS_IO_WRITE, ...)

As a result, any concurrent getattr with non-zero flags (such as
orangefs_getattr() or orangefs_file_llseek()) fetches the pre-write size
from the server and clobbers inode->i_size, which also causes
orangefs_writepage_locked() (or a subsequent batch in
orangefs_writepages_work()) to clamp the write length to the stale size and
truncate the buffered write.

Should these checks also test mapping_tagged(inode->i_mapping,
PAGECACHE_TAG_WRITEBACK) (or mapping_writably_mapped() / a helper covering
both PAGECACHE_TAG_DIRTY and PAGECACHE_TAG_WRITEBACK)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113083336.15152-1-jack@suse.cz?part=3