Re: [PATCH 1/2] ext4: use fsdata to track inline data write state

[Jan Kara]

On Tue 30-06-26 15:28:11, Aditya Srivastava wrote:
> From: Aditya Prakash Srivastava <aditya.ansh182@xxxxxxxxx>
> 
> Instead of checking the live inode state (`ext4_has_inline_data(inode)`
> and `ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)`) in the
> write_end handlers, use the `fsdata` parameter of the address space
> operations to explicitly pass down the state in which `write_begin`
> prepared the write.
> 
> A concurrent thread (such as `ext4_page_mkwrite()`) can convert the
> inline data to an extent between `write_begin` and `write_end`. If this
> happens, the write_end handlers would previously miss the inline
> write_end path and fall through to extent-based write_end logic. However,
> since block buffers were never allocated in `write_begin`, this resulted
> in NULL pointer dereferences or data loss because `folio_buffers(folio)`
> was NULL.
> 
> By defining `EXT4_WRITE_DATA_INLINE` (3) and communicating this state via
> `fsdata`:
> 1) `ext4_write_begin()` and `ext4_da_write_begin()` explicitly set
>    `*fsdata` to `EXT4_WRITE_DATA_INLINE` when an inline write is
>    successfully prepared.
> 2) `ext4_write_end()`, `ext4_journalled_write_end()`, and
>    `ext4_da_write_end()` rely solely on `fsdata` / `write_mode` to
>    invoke `ext4_write_inline_data_end()`.
> 
> This removes the crude race fallbacks and makes the write_end
> determination unambiguous, simple, and clean.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@xxxxxxxxx>
> ---
>  fs/ext4/ext4.h  |  1 +
>  fs/ext4/inode.c | 22 +++++++++++++---------
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b37c136ea3ab..521bd5d6321c 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3138,6 +3138,7 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
>  void ext4_set_inode_mapping_order(struct inode *inode);
>  #define FALL_BACK_TO_NONDELALLOC 1
>  #define CONVERT_INLINE_DATA	 2
> +#define EXT4_WRITE_DATA_INLINE	 3
>  
>  typedef enum {
>  	EXT4_IGET_NORMAL =	0,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ce99807c5f5b..e2e8ac5fb8d8 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1302,6 +1302,9 @@ static int ext4_write_begin(const struct kiocb *iocb,
>  	if (unlikely(ret))
>  		return ret;
>  
> +	if (fsdata && *fsdata != (void *)FALL_BACK_TO_NONDELALLOC)
> +		*fsdata = NULL;

This is pointless. *fsdata is guaranteed to be NULL on entry to
->write_begin from generic code, ext4 calls ext4_write_begin() from one
place and there it has FALL_BACK_TO_NONDELALLOC value.

> +
>  	trace_ext4_write_begin(inode, pos, len);
>  	/*
>  	 * Reserve one block more for addition to orphan list in case
> @@ -1316,8 +1319,11 @@ static int ext4_write_begin(const struct kiocb *iocb,
>  						    foliop);
>  		if (ret < 0)
>  			return ret;
> -		if (ret == 1)
> +		if (ret == 1) {
> +			if (fsdata)

fsdata is guaranteed to be non-NULL so no need to check for it here.

> +				*fsdata = (void *)EXT4_WRITE_DATA_INLINE;
>  			return 0;
> +		}
>  	}
>  
>  	/*
> @@ -1450,8 +1456,7 @@ static int ext4_write_end(const struct kiocb *iocb,
>  
>  	trace_ext4_write_end(inode, pos, len, copied);
>  
> -	if (ext4_has_inline_data(inode) &&
> -	    ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
> +	if (fsdata == (void *)EXT4_WRITE_DATA_INLINE)
>  		return ext4_write_inline_data_end(inode, pos, len, copied,
>  						  folio);

OK, but if we race like:
  CPU1						CPU2
  ext4_try_to_write_inline_data()
						ext4_page_mkwrite()
  ext4_write_inline_data_end()

Then ext4_write_inline_data_end() will now hit:

  BUG_ON(!ext4_has_inline_data(inode));

with much higher probability. You fix that in patch 2 but generally we try
hard to avoid such breaking points in git history. So please fold patch 2
into this one to keep things working all the time.

								Honza

>  
> @@ -1560,8 +1565,7 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
>  
>  	BUG_ON(!ext4_handle_valid(handle));
>  
> -	if (ext4_has_inline_data(inode) &&
> -	    ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
> +	if (fsdata == (void *)EXT4_WRITE_DATA_INLINE)
>  		return ext4_write_inline_data_end(inode, pos, len, copied,
>  						  folio);
>  
> @@ -3161,8 +3165,10 @@ static int ext4_da_write_begin(const struct kiocb *iocb,
>  						     foliop, fsdata, true);
>  		if (ret < 0)
>  			return ret;
> -		if (ret == 1)
> +		if (ret == 1) {
> +			*fsdata = (void *)EXT4_WRITE_DATA_INLINE;
>  			return 0;
> +		}
>  	}
>  
>  retry:
> @@ -3299,9 +3305,7 @@ static int ext4_da_write_end(const struct kiocb *iocb,
>  
>  	trace_ext4_da_write_end(inode, pos, len, copied);
>  
> -	if (write_mode != CONVERT_INLINE_DATA &&
> -	    ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA) &&
> -	    ext4_has_inline_data(inode))
> +	if (write_mode == EXT4_WRITE_DATA_INLINE)
>  		return ext4_write_inline_data_end(inode, pos, len, copied,
>  						  folio);
>  
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR