Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 16:48:35 +0200
From:   Jan Kara <jack@suse.cz>
To:     Zhihao Cheng <chengzhihao1@huawei.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com
Subject: Re: [PATCH] ext4: Fix unttached inode after power cut with orphan
 file feature enabled
Message-ID: <20230628144835.fqhmbpxoj6fee762@quack3>
References: <20230628132011.650383-1-chengzhihao1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230628132011.650383-1-chengzhihao1@huawei.com>
Precedence: bulk

On Wed 28-06-23 21:20:11, Zhihao Cheng wrote:
> Running generic/475(filesystem consistent tests after power cut) could
> easily trigger unattached inode error while doing fsck:
>   Unattached zero-length inode 39405.  Clear? no
> 
>   Unattached inode 39405
>   Connect to /lost+found? no
> 
> Above inconsistence is caused by following process:
>        P1                       P2
> ext4_create
>  inode = ext4_new_inode_start_handle  // itable records nlink=1
>  ext4_add_nondir
>    err = ext4_add_entry  // ENOSPC
>     ext4_append
>      ext4_bread
>       ext4_getblk
>        ext4_map_blocks // returns ENOSPC
>    drop_nlink(inode) // won't be updated into disk inode
>    ext4_orphan_add(handle, inode)
>     ext4_orphan_file_add
>  ext4_journal_stop(handle)
> 		      jbd2_journal_commit_transaction // commit success
>               >> power cut <<
> ext4_fill_super
>  ext4_load_and_init_journal   // itable records nlink=1
>  ext4_orphan_cleanup
>   ext4_process_orphan
>    if (inode->i_nlink)        // true, inode won't be deleted
> 
> Then, allocated inode will be reserved on disk and corresponds to no
> dentries, so e2fsck reports 'unattached inode' problem.
> 
> The problem won't happen if orphan file feature is disabled, because
> ext4_orphan_add() will update disk inode in orphan list mode. There
> are several places not updating disk inode while putting inode into
> orphan area, such as ext4_add_nondir(), ext4_symlink() and whiteout
> in ext4_rename(). Fix it by updating inode into disk in all error
> branches of these places.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217605
> Fixes: 02f310fcf47f ("ext4: Speedup ext4 orphan inode handling")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>

Nice catch. Thanks for fixing this. Feel free to add:

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

								Honza

> ---
>  fs/ext4/namei.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 0caf6c730ce3..6bcc3770ee19 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2799,6 +2799,7 @@ static int ext4_add_nondir(handle_t *handle,
>  		return err;
>  	}
>  	drop_nlink(inode);
> +	ext4_mark_inode_dirty(handle, inode);
>  	ext4_orphan_add(handle, inode);
>  	unlock_new_inode(inode);
>  	return err;
> @@ -3436,6 +3437,7 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  
>  err_drop_inode:
>  	clear_nlink(inode);
> +	ext4_mark_inode_dirty(handle, inode);
>  	ext4_orphan_add(handle, inode);
>  	unlock_new_inode(inode);
>  	if (handle)
> @@ -4021,6 +4023,7 @@ static int ext4_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>  			ext4_resetent(handle, &old,
>  				      old.inode->i_ino, old_file_type);
>  			drop_nlink(whiteout);
> +			ext4_mark_inode_dirty(handle, whiteout);
>  			ext4_orphan_add(handle, whiteout);
>  		}
>  		unlock_new_inode(whiteout);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR