[PATCH v2] ext4: use fsdata to track inline data write state and fix race

[PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Aditya Srivastava]

From: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>

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.

Define EXT4_WRITE_DATA_INLINE (3) and communicate 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().

Furthermore, during a buffered write, ext4_write_inline_data_end()
acquires the xattr lock after preparing the write. If a concurrent
page fault (ext4_page_mkwrite()) converts the inline data to an extent
after the write_end handlers check the state but before
ext4_write_inline_data_end() acquires the xattr write lock, the
subsequent check will trigger a kernel panic via
BUG_ON(!ext4_has_inline_data(inode)).

Replace the BUG_ON check in ext4_write_inline_data_end() with a graceful
error-handling retry path. If the inline data is cleared after locking
the xattr, we safely release all resources (releasing iloc.bh,
unlocking/putting the folio, stopping the active journal transaction
handle) and return 0 (VFS retry) to let the generic write path retry
the operation safely.

Reported-by: syzbot+0c89d865531d053abb2d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0c89d865531d053abb2d
Fixes: 3fdcfb668fd7 ("ext4: add journalled write support for inline data")
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
---
Changes in v2:
- Folded the BUG_ON fix from the second patch into the first one to
  ensure bisectability across git history, as suggested by Jan Kara.
- Removed the pointless initialization `*fsdata = NULL` on entry to
  `ext4_write_begin()`.
- Removed the redundant check `if (fsdata)` in `ext4_write_begin()`.

 fs/ext4/ext4.h   |  1 +
 fs/ext4/inline.c | 14 +++++++++++++-
 fs/ext4/inode.c  | 18 +++++++++---------
 3 files changed, 23 insertions(+), 10 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/inline.c b/fs/ext4/inline.c
index 8045e4ff270c..cfd591dc1d9c 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -812,7 +812,19 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
 			goto out;
 		}
 		ext4_write_lock_xattr(inode, &no_expand);
-		BUG_ON(!ext4_has_inline_data(inode));
+		/*
+		 * We could have raced with ext4_page_mkwrite() converting
+		 * the inode and clearing the inline data flag, so we just
+		 * release resources and retry the whole write.
+		 */
+		if (unlikely(!ext4_has_inline_data(inode))) {
+			ext4_write_unlock_xattr(inode, &no_expand);
+			brelse(iloc.bh);
+			folio_unlock(folio);
+			folio_put(folio);
+			ext4_journal_stop(handle);
+			return 0;
+		}
 
 		/*
 		 * ei->i_inline_off may have changed since
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ce99807c5f5b..d3d9e1999670 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1316,8 +1316,10 @@ static int ext4_write_begin(const struct kiocb *iocb,
 						    foliop);
 		if (ret < 0)
 			return ret;
-		if (ret == 1)
+		if (ret == 1) {
+			*fsdata = (void *)EXT4_WRITE_DATA_INLINE;
 			return 0;
+		}
 	}
 
 	/*
@@ -1450,8 +1452,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);
 
@@ -1560,8 +1561,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 +3161,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 +3301,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

Re: [PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Jan Kara]

On Thu 02-07-26 05:07:24, Aditya Srivastava wrote:
> From: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
> 
> 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.
> 
> Define EXT4_WRITE_DATA_INLINE (3) and communicate 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().
> 
> Furthermore, during a buffered write, ext4_write_inline_data_end()
> acquires the xattr lock after preparing the write. If a concurrent
> page fault (ext4_page_mkwrite()) converts the inline data to an extent
> after the write_end handlers check the state but before
> ext4_write_inline_data_end() acquires the xattr write lock, the
> subsequent check will trigger a kernel panic via
> BUG_ON(!ext4_has_inline_data(inode)).
> 
> Replace the BUG_ON check in ext4_write_inline_data_end() with a graceful
> error-handling retry path. If the inline data is cleared after locking
> the xattr, we safely release all resources (releasing iloc.bh,
> unlocking/putting the folio, stopping the active journal transaction
> handle) and return 0 (VFS retry) to let the generic write path retry
> the operation safely.
> 
> Reported-by: syzbot+0c89d865531d053abb2d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0c89d865531d053abb2d
> Fixes: 3fdcfb668fd7 ("ext4: add journalled write support for inline data")
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> Changes in v2:
> - Folded the BUG_ON fix from the second patch into the first one to
>   ensure bisectability across git history, as suggested by Jan Kara.
> - Removed the pointless initialization `*fsdata = NULL` on entry to
>   `ext4_write_begin()`.
> - Removed the redundant check `if (fsdata)` in `ext4_write_begin()`.
> 
>  fs/ext4/ext4.h   |  1 +
>  fs/ext4/inline.c | 14 +++++++++++++-
>  fs/ext4/inode.c  | 18 +++++++++---------
>  3 files changed, 23 insertions(+), 10 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/inline.c b/fs/ext4/inline.c
> index 8045e4ff270c..cfd591dc1d9c 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -812,7 +812,19 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
>  			goto out;
>  		}
>  		ext4_write_lock_xattr(inode, &no_expand);
> -		BUG_ON(!ext4_has_inline_data(inode));
> +		/*
> +		 * We could have raced with ext4_page_mkwrite() converting
> +		 * the inode and clearing the inline data flag, so we just
> +		 * release resources and retry the whole write.
> +		 */
> +		if (unlikely(!ext4_has_inline_data(inode))) {
> +			ext4_write_unlock_xattr(inode, &no_expand);
> +			brelse(iloc.bh);
> +			folio_unlock(folio);
> +			folio_put(folio);
> +			ext4_journal_stop(handle);
> +			return 0;
> +		}
>  
>  		/*
>  		 * ei->i_inline_off may have changed since
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ce99807c5f5b..d3d9e1999670 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1316,8 +1316,10 @@ static int ext4_write_begin(const struct kiocb *iocb,
>  						    foliop);
>  		if (ret < 0)
>  			return ret;
> -		if (ret == 1)
> +		if (ret == 1) {
> +			*fsdata = (void *)EXT4_WRITE_DATA_INLINE;
>  			return 0;
> +		}
>  	}
>  
>  	/*
> @@ -1450,8 +1452,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);
>  
> @@ -1560,8 +1561,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 +3161,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 +3301,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@suse.com>
SUSE Labs, CR

Re: [PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Aditya Prakash Srivastava]

Hi Jan,

I have been thinking about the VFS retry case where if we do
not reset the fsdata, we might end up in an infinite loop.

See below

________________________________

Phase 1: Preparing the inline write (Thread 1)

VFS entry: Userspace calls write(). The kernel enters
generic_perform_write().

Setup: VFS defines void *fsdata = NULL on the stack.

Begin: ext4_write_begin() is invoked.

The file is inline, so it prepares an inline write.

It sets *fsdata = (void *)EXT4_WRITE_DATA_INLINE.

It returns success (0).

Data copy: VFS copies the userspace data into the page/folio.

________________________________

Phase 2: The race and inline conversion (Thread 2)

Concurrent conversion: While Thread 1 is preparing the write,
Thread 2 triggers a page fault (ext4_page_mkwrite()).

Conversion complete: Thread 2 converts the inode from inline
data to standard extents, clearing
EXT4_STATE_MAY_INLINE_DATA and the inode's inline-data
flag.

________________________________

Phase 3: write_end() and the first retry (Thread 1)

End attempt: VFS calls ext4_write_end().

Inline path: ext4_write_end() sees
fsdata == EXT4_WRITE_DATA_INLINE and routes the write to
ext4_write_inline_data_end().

Conversion detected: ext4_write_inline_data_end() acquires
the xattr lock and checks ext4_has_inline_data(inode).

Since Thread 2 converted the file, the check returns
false.

Retry requested: ext4_write_inline_data_end() releases its
locks/resources and returns 0, telling the VFS that the
write should be retried from the beginning.

________________________________

Phase 4: The VFS retry and the bug (Thread 1)

Retry: generic_perform_write() receives 0 and executes
goto retry;.

Stale fsdata: The retry: label is after the
fsdata = NULL initialization. Therefore:

fsdata is not reset to NULL.

It still contains EXT4_WRITE_DATA_INLINE.

Second begin: ext4_write_begin() is called again.

Since *fsdata = NULL was removed from
ext4_write_begin(), *fsdata is left unchanged and
still contains EXT4_WRITE_DATA_INLINE.

Since the file is no longer inline,
ext4_write_begin() prepares a normal extent-based
write.

Second end: VFS calls ext4_write_end().

It checks whether
fsdata == EXT4_WRITE_DATA_INLINE.

Although this write was prepared as an extent write,
the check is still true because fsdata is stale.

Loop closes: ext4_write_end() incorrectly routes the write
to ext4_write_inline_data_end() again.

It acquires the xattr lock, sees that the inode no
longer contains inline data, releases its resources,
and returns 0 again.

Infinite loop: VFS jumps back to retry:. Since fsdata is
never cleared, the same sequence repeats indefinitely.
________________________________

The proposed fix from my side is to reintroduce setting
fsdata = NULL in the beginning of ext4_write_begin,
of course we should be careful not to overwrite it if it
has some other value. Sample code like below

if (unlikely(ret))
return ret;

/*
 * Reset fsdata for a vfs retry case
*/
if (*fsdata != (void *)FALL_BACK_TO_NONDELALLOC)
*fsdata = NULL;

trace_ext4_write_begin(inode, pos, len);
/*
 * Reserve one block more for addition to orphan list in case

This should take care of the retry case not getting into
an infinite loop.
What do you think? I shall send you a revised version if we
agree on this.

Thanks,
Aditya

On Thu, Jul 2, 2026 at 4:05 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 02-07-26 05:07:24, Aditya Srivastava wrote:
> > From: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
> >
> > 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.
> >
> > Define EXT4_WRITE_DATA_INLINE (3) and communicate 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().
> >
> > Furthermore, during a buffered write, ext4_write_inline_data_end()
> > acquires the xattr lock after preparing the write. If a concurrent
> > page fault (ext4_page_mkwrite()) converts the inline data to an extent
> > after the write_end handlers check the state but before
> > ext4_write_inline_data_end() acquires the xattr write lock, the
> > subsequent check will trigger a kernel panic via
> > BUG_ON(!ext4_has_inline_data(inode)).
> >
> > Replace the BUG_ON check in ext4_write_inline_data_end() with a graceful
> > error-handling retry path. If the inline data is cleared after locking
> > the xattr, we safely release all resources (releasing iloc.bh,
> > unlocking/putting the folio, stopping the active journal transaction
> > handle) and return 0 (VFS retry) to let the generic write path retry
> > the operation safely.
> >
> > Reported-by: syzbot+0c89d865531d053abb2d@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=0c89d865531d053abb2d
> > Fixes: 3fdcfb668fd7 ("ext4: add journalled write support for inline data")
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
>
> Looks good! Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
>                                                                 Honza
>
> > ---
> > Changes in v2:
> > - Folded the BUG_ON fix from the second patch into the first one to
> >   ensure bisectability across git history, as suggested by Jan Kara.
> > - Removed the pointless initialization `*fsdata = NULL` on entry to
> >   `ext4_write_begin()`.
> > - Removed the redundant check `if (fsdata)` in `ext4_write_begin()`.
> >
> >  fs/ext4/ext4.h   |  1 +
> >  fs/ext4/inline.c | 14 +++++++++++++-
> >  fs/ext4/inode.c  | 18 +++++++++---------
> >  3 files changed, 23 insertions(+), 10 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/inline.c b/fs/ext4/inline.c
> > index 8045e4ff270c..cfd591dc1d9c 100644
> > --- a/fs/ext4/inline.c
> > +++ b/fs/ext4/inline.c
> > @@ -812,7 +812,19 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
> >                       goto out;
> >               }
> >               ext4_write_lock_xattr(inode, &no_expand);
> > -             BUG_ON(!ext4_has_inline_data(inode));
> > +             /*
> > +              * We could have raced with ext4_page_mkwrite() converting
> > +              * the inode and clearing the inline data flag, so we just
> > +              * release resources and retry the whole write.
> > +              */
> > +             if (unlikely(!ext4_has_inline_data(inode))) {
> > +                     ext4_write_unlock_xattr(inode, &no_expand);
> > +                     brelse(iloc.bh);
> > +                     folio_unlock(folio);
> > +                     folio_put(folio);
> > +                     ext4_journal_stop(handle);
> > +                     return 0;
> > +             }
> >
> >               /*
> >                * ei->i_inline_off may have changed since
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index ce99807c5f5b..d3d9e1999670 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -1316,8 +1316,10 @@ static int ext4_write_begin(const struct kiocb *iocb,
> >                                                   foliop);
> >               if (ret < 0)
> >                       return ret;
> > -             if (ret == 1)
> > +             if (ret == 1) {
> > +                     *fsdata = (void *)EXT4_WRITE_DATA_INLINE;
> >                       return 0;
> > +             }
> >       }
> >
> >       /*
> > @@ -1450,8 +1452,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);
> >
> > @@ -1560,8 +1561,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 +3161,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 +3301,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@suse.com>
> SUSE Labs, CR

Re: [PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Jan Kara]

Hi Aditya!

On Thu 02-07-26 16:19:56, Aditya Prakash Srivastava wrote:
> I have been thinking about the VFS retry case where if we do
> not reset the fsdata, we might end up in an infinite loop.

Indeed, good spotting. I have also realized that after your patch
CONVERT_INLINE_DATA is effectively unused and can be deleted along with
passing fsdata to ext4_generic_write_inline_data() and
ext4_da_convert_inline_data_to_extent(). So that would be a good cleanup
patch on top of your fix.

> The proposed fix from my side is to reintroduce setting
> fsdata = NULL in the beginning of ext4_write_begin,
> of course we should be careful not to overwrite it if it
> has some other value. Sample code like below
> 
> if (unlikely(ret))
> return ret;
> 
> /*
>  * Reset fsdata for a vfs retry case
> */
> if (*fsdata != (void *)FALL_BACK_TO_NONDELALLOC)
> *fsdata = NULL;

Hum, thinking more about this I think we should use fsdata more as flags
than enum. So ext4_write_begin() should be doing

  *fsdata = (void *)((unsigned long)*fsdata | EXT4_WRITE_DATA_INLINE);

and ext4_write_end() should be doing (unconditionally on exit):

  *fsdata = (void *)((unsigned long)*fsdata & ~EXT4_WRITE_DATA_INLINE);

to unroll the state back. Otherwise the interactions with
FALL_BACK_TO_NONDELALLOC and delalloc path get too tedious to verify and
they *are* in fact independent states of the write path.

								Honza

> 
> trace_ext4_write_begin(inode, pos, len);
> /*
>  * Reserve one block more for addition to orphan list in case
> 
> This should take care of the retry case not getting into
> an infinite loop.
> What do you think? I shall send you a revised version if we
> agree on this.
> 
> Thanks,
> Aditya
> 
> On Thu, Jul 2, 2026 at 4:05 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 02-07-26 05:07:24, Aditya Srivastava wrote:
> > > From: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
> > >
> > > 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.
> > >
> > > Define EXT4_WRITE_DATA_INLINE (3) and communicate 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().
> > >
> > > Furthermore, during a buffered write, ext4_write_inline_data_end()
> > > acquires the xattr lock after preparing the write. If a concurrent
> > > page fault (ext4_page_mkwrite()) converts the inline data to an extent
> > > after the write_end handlers check the state but before
> > > ext4_write_inline_data_end() acquires the xattr write lock, the
> > > subsequent check will trigger a kernel panic via
> > > BUG_ON(!ext4_has_inline_data(inode)).
> > >
> > > Replace the BUG_ON check in ext4_write_inline_data_end() with a graceful
> > > error-handling retry path. If the inline data is cleared after locking
> > > the xattr, we safely release all resources (releasing iloc.bh,
> > > unlocking/putting the folio, stopping the active journal transaction
> > > handle) and return 0 (VFS retry) to let the generic write path retry
> > > the operation safely.
> > >
> > > Reported-by: syzbot+0c89d865531d053abb2d@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=0c89d865531d053abb2d
> > > Fixes: 3fdcfb668fd7 ("ext4: add journalled write support for inline data")
> > > Suggested-by: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
> >
> > Looks good! Feel free to add:
> >
> > Reviewed-by: Jan Kara <jack@suse.cz>
> >
> >                                                                 Honza
> >
> > > ---
> > > Changes in v2:
> > > - Folded the BUG_ON fix from the second patch into the first one to
> > >   ensure bisectability across git history, as suggested by Jan Kara.
> > > - Removed the pointless initialization `*fsdata = NULL` on entry to
> > >   `ext4_write_begin()`.
> > > - Removed the redundant check `if (fsdata)` in `ext4_write_begin()`.
> > >
> > >  fs/ext4/ext4.h   |  1 +
> > >  fs/ext4/inline.c | 14 +++++++++++++-
> > >  fs/ext4/inode.c  | 18 +++++++++---------
> > >  3 files changed, 23 insertions(+), 10 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/inline.c b/fs/ext4/inline.c
> > > index 8045e4ff270c..cfd591dc1d9c 100644
> > > --- a/fs/ext4/inline.c
> > > +++ b/fs/ext4/inline.c
> > > @@ -812,7 +812,19 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
> > >                       goto out;
> > >               }
> > >               ext4_write_lock_xattr(inode, &no_expand);
> > > -             BUG_ON(!ext4_has_inline_data(inode));
> > > +             /*
> > > +              * We could have raced with ext4_page_mkwrite() converting
> > > +              * the inode and clearing the inline data flag, so we just
> > > +              * release resources and retry the whole write.
> > > +              */
> > > +             if (unlikely(!ext4_has_inline_data(inode))) {
> > > +                     ext4_write_unlock_xattr(inode, &no_expand);
> > > +                     brelse(iloc.bh);
> > > +                     folio_unlock(folio);
> > > +                     folio_put(folio);
> > > +                     ext4_journal_stop(handle);
> > > +                     return 0;
> > > +             }
> > >
> > >               /*
> > >                * ei->i_inline_off may have changed since
> > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > > index ce99807c5f5b..d3d9e1999670 100644
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -1316,8 +1316,10 @@ static int ext4_write_begin(const struct kiocb *iocb,
> > >                                                   foliop);
> > >               if (ret < 0)
> > >                       return ret;
> > > -             if (ret == 1)
> > > +             if (ret == 1) {
> > > +                     *fsdata = (void *)EXT4_WRITE_DATA_INLINE;
> > >                       return 0;
> > > +             }
> > >       }
> > >
> > >       /*
> > > @@ -1450,8 +1452,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);
> > >
> > > @@ -1560,8 +1561,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 +3161,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 +3301,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@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Aditya Prakash Srivastava]

Thank You Jan for your valuable inputs. As you pointed out,
CONVERT_INLINE_DATA indeed becomes unused. I will be doing
that cleanup once we sort this one out.

Coming back to this patch, I will incorporate the suggestions, test it
and will send the next revision of this patch. I agree that
interactions with FALL_BACK_TO_NONDELALLOC introduce
unnecessary cross-over of two independent paths and should be
avoided altogether.

Thanks,
Aditya

On Thu, Jul 2, 2026 at 5:04 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi Aditya!
>
> On Thu 02-07-26 16:19:56, Aditya Prakash Srivastava wrote:
> > I have been thinking about the VFS retry case where if we do
> > not reset the fsdata, we might end up in an infinite loop.
>
> Indeed, good spotting. I have also realized that after your patch
> CONVERT_INLINE_DATA is effectively unused and can be deleted along with
> passing fsdata to ext4_generic_write_inline_data() and
> ext4_da_convert_inline_data_to_extent(). So that would be a good cleanup
> patch on top of your fix.
>
> > The proposed fix from my side is to reintroduce setting
> > fsdata = NULL in the beginning of ext4_write_begin,
> > of course we should be careful not to overwrite it if it
> > has some other value. Sample code like below
> >
> > if (unlikely(ret))
> > return ret;
> >
> > /*
> >  * Reset fsdata for a vfs retry case
> > */
> > if (*fsdata != (void *)FALL_BACK_TO_NONDELALLOC)
> > *fsdata = NULL;
>
> Hum, thinking more about this I think we should use fsdata more as flags
> than enum. So ext4_write_begin() should be doing
>
>   *fsdata = (void *)((unsigned long)*fsdata | EXT4_WRITE_DATA_INLINE);
>
> and ext4_write_end() should be doing (unconditionally on exit):
>
>   *fsdata = (void *)((unsigned long)*fsdata & ~EXT4_WRITE_DATA_INLINE);
>
> to unroll the state back. Otherwise the interactions with
> FALL_BACK_TO_NONDELALLOC and delalloc path get too tedious to verify and
> they *are* in fact independent states of the write path.
>
>                                                                 Honza
>
> >
> > trace_ext4_write_begin(inode, pos, len);
> > /*
> >  * Reserve one block more for addition to orphan list in case
> >
> > This should take care of the retry case not getting into
> > an infinite loop.
> > What do you think? I shall send you a revised version if we
> > agree on this.
> >
> > Thanks,
> > Aditya
> >
> > On Thu, Jul 2, 2026 at 4:05 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Thu 02-07-26 05:07:24, Aditya Srivastava wrote:
> > > > From: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
> > > >
> > > > 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.
> > > >
> > > > Define EXT4_WRITE_DATA_INLINE (3) and communicate 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().
> > > >
> > > > Furthermore, during a buffered write, ext4_write_inline_data_end()
> > > > acquires the xattr lock after preparing the write. If a concurrent
> > > > page fault (ext4_page_mkwrite()) converts the inline data to an extent
> > > > after the write_end handlers check the state but before
> > > > ext4_write_inline_data_end() acquires the xattr write lock, the
> > > > subsequent check will trigger a kernel panic via
> > > > BUG_ON(!ext4_has_inline_data(inode)).
> > > >
> > > > Replace the BUG_ON check in ext4_write_inline_data_end() with a graceful
> > > > error-handling retry path. If the inline data is cleared after locking
> > > > the xattr, we safely release all resources (releasing iloc.bh,
> > > > unlocking/putting the folio, stopping the active journal transaction
> > > > handle) and return 0 (VFS retry) to let the generic write path retry
> > > > the operation safely.
> > > >
> > > > Reported-by: syzbot+0c89d865531d053abb2d@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=0c89d865531d053abb2d
> > > > Fixes: 3fdcfb668fd7 ("ext4: add journalled write support for inline data")
> > > > Suggested-by: Jan Kara <jack@suse.cz>
> > > > Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@gmail.com>
> > >
> > > Looks good! Feel free to add:
> > >
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > >
> > >                                                                 Honza
> > >
> > > > ---
> > > > Changes in v2:
> > > > - Folded the BUG_ON fix from the second patch into the first one to
> > > >   ensure bisectability across git history, as suggested by Jan Kara.
> > > > - Removed the pointless initialization `*fsdata = NULL` on entry to
> > > >   `ext4_write_begin()`.
> > > > - Removed the redundant check `if (fsdata)` in `ext4_write_begin()`.
> > > >
> > > >  fs/ext4/ext4.h   |  1 +
> > > >  fs/ext4/inline.c | 14 +++++++++++++-
> > > >  fs/ext4/inode.c  | 18 +++++++++---------
> > > >  3 files changed, 23 insertions(+), 10 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/inline.c b/fs/ext4/inline.c
> > > > index 8045e4ff270c..cfd591dc1d9c 100644
> > > > --- a/fs/ext4/inline.c
> > > > +++ b/fs/ext4/inline.c
> > > > @@ -812,7 +812,19 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
> > > >                       goto out;
> > > >               }
> > > >               ext4_write_lock_xattr(inode, &no_expand);
> > > > -             BUG_ON(!ext4_has_inline_data(inode));
> > > > +             /*
> > > > +              * We could have raced with ext4_page_mkwrite() converting
> > > > +              * the inode and clearing the inline data flag, so we just
> > > > +              * release resources and retry the whole write.
> > > > +              */
> > > > +             if (unlikely(!ext4_has_inline_data(inode))) {
> > > > +                     ext4_write_unlock_xattr(inode, &no_expand);
> > > > +                     brelse(iloc.bh);
> > > > +                     folio_unlock(folio);
> > > > +                     folio_put(folio);
> > > > +                     ext4_journal_stop(handle);
> > > > +                     return 0;
> > > > +             }
> > > >
> > > >               /*
> > > >                * ei->i_inline_off may have changed since
> > > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > > > index ce99807c5f5b..d3d9e1999670 100644
> > > > --- a/fs/ext4/inode.c
> > > > +++ b/fs/ext4/inode.c
> > > > @@ -1316,8 +1316,10 @@ static int ext4_write_begin(const struct kiocb *iocb,
> > > >                                                   foliop);
> > > >               if (ret < 0)
> > > >                       return ret;
> > > > -             if (ret == 1)
> > > > +             if (ret == 1) {
> > > > +                     *fsdata = (void *)EXT4_WRITE_DATA_INLINE;
> > > >                       return 0;
> > > > +             }
> > > >       }
> > > >
> > > >       /*
> > > > @@ -1450,8 +1452,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);
> > > >
> > > > @@ -1560,8 +1561,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 +3161,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 +3301,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@suse.com>
> > > SUSE Labs, CR
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Aditya Prakash Srivastava]

Hi Jan,

Thought a bit more about it. Treating fsdata as
bitwise flags rather than mutually exclusive enums is a much
cleaner and more robust design. It completely decouples the
different write path states.

To implement this, I will define:

#define FALL_BACK_TO_NONDELALLOC 1
#define CONVERT_INLINE_DATA 2
#define EXT4_WRITE_DATA_INLINE 4

This ensures that the flags are fully independent and do not
overlap.

Regarding unrolling the state on exit, there is a slight
asymmetry in the VFS interface. ext4_write_begin() receives
fsdata as a pointer-to-pointer (void **fsdata), while
ext4_write_end() receives it by value (void *fsdata).

Because write_end() receives fsdata by value, modifying it in
the write_end handlers has no effect on the caller's
(generic_perform_write()) local stack variable. As a result, a
stale EXT4_WRITE_DATA_INLINE bit would remain in the VFS across
a retry.

To handle this cleanly within the constraints of the VFS
interface, I propose clearing the stale bit on entry to
ext4_write_begin():

static int ext4_write_begin(..., void **fsdata)
{
...

/* Clear any stale inline flag from a previous retry. */
*fsdata = (void *)((unsigned long)*fsdata &
  ~EXT4_WRITE_DATA_INLINE);

...
}

This ensures that:

any stale inline flag from a previous retry is cleared from
the VFS stack variable before the next attempt;

the FALL_BACK_TO_NONDELALLOC bit remains preserved; and

if the retry prepares an inline write, ext4_write_begin()
simply sets the bit again.

Please let me know whether this write_begin unrolling approach
looks good to you, and I'll submit the v3 series shortly.

Thanks,
Aditya

Re: [PATCH v2] ext4: use fsdata to track inline data write state and fix race

[Jan Kara]

Hi Aditya,

On Thu 02-07-26 18:15:21, Aditya Prakash Srivastava wrote:
> Thought a bit more about it. Treating fsdata as
> bitwise flags rather than mutually exclusive enums is a much
> cleaner and more robust design. It completely decouples the
> different write path states.
> 
> To implement this, I will define:
> 
> #define FALL_BACK_TO_NONDELALLOC 1
> #define CONVERT_INLINE_DATA 2
> #define EXT4_WRITE_DATA_INLINE 4
> 
> This ensures that the flags are fully independent and do not
> overlap.
> 
> Regarding unrolling the state on exit, there is a slight
> asymmetry in the VFS interface. ext4_write_begin() receives
> fsdata as a pointer-to-pointer (void **fsdata), while
> ext4_write_end() receives it by value (void *fsdata).
> 
> Because write_end() receives fsdata by value, modifying it in
> the write_end handlers has no effect on the caller's
> (generic_perform_write()) local stack variable. As a result, a
> stale EXT4_WRITE_DATA_INLINE bit would remain in the VFS across
> a retry.

Right, my bad.

> To handle this cleanly within the constraints of the VFS
> interface, I propose clearing the stale bit on entry to
> ext4_write_begin():
> 
> static int ext4_write_begin(..., void **fsdata)
> {
> ...
> 
> /* Clear any stale inline flag from a previous retry. */
> *fsdata = (void *)((unsigned long)*fsdata &
>   ~EXT4_WRITE_DATA_INLINE);
> 
> ...
> }
> 
> This ensures that:
> 
> any stale inline flag from a previous retry is cleared from
> the VFS stack variable before the next attempt;
> 
> the FALL_BACK_TO_NONDELALLOC bit remains preserved; and
> 
> if the retry prepares an inline write, ext4_write_begin()
> simply sets the bit again.
> 
> Please let me know whether this write_begin unrolling approach
> looks good to you, and I'll submit the v3 series shortly.

Yes, that should work and should be clean.

								Honza

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