From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 767ED3D3B8 for ; Wed, 15 Jan 2025 02:12:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736907174; cv=none; b=XTWGywt1U1DXgAEJAID6YpH0Y/XoNJ7SIsO2T7/856EKpec4JkwX0iQc4Pf528Gii6E0rB51eWLv50L0e7NkhAxu7jBGymQhqZbMu49DNTHuVmTDVljhD2eza/72Sk7fXX8hlGV+CnbZ82VPzwJiooxXhBF3A4zrq0fAh0YogkI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736907174; c=relaxed/simple; bh=YctuTXxmn9FfsyTsjD9c+mulfgwi9HKPjvELcWvQ0i4=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=DIYHBjEl5bCtP3gvx+/dzSPPeYhfURsleaIv9muDtgYRlJyjK4YE7yRDBTXa9/OKBN7zq8MISWccE+Nkwbf8BPcqPyzzs20UnXwTStLKgO1mPzDMGoGUv3wVfhkUxrEWfe0dxFTHyGDXP/FUSKsnt3xr8OBFH4iO5oiHJI4WzFM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=uJgT40c3; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="uJgT40c3" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 267CCC4CEDD; Wed, 15 Jan 2025 02:12:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736907173; bh=YctuTXxmn9FfsyTsjD9c+mulfgwi9HKPjvELcWvQ0i4=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=uJgT40c3xNuDU/fxvgrG77+L3dW7tMzVz/UNepOkQ8lRSdbxktbQr/NYuwjLgkcBy uHZRAjXK72LmwJ0gNXZHgefhidyingrgcSmmmeT6UM1rdZPJliGIubT+s4UI2GefMz VaCRRSbpmWvNHPZlCJ9A7mDWvbDJJ63/XurvgHIQIL8+vP3EtKaydOOODKZrl4phIX bIDeVBeMkmTz+b7pOEyZzNYAndKxtwLoZFuGe2Z3ycfx+F0d5pfdjaxVBs7cq1ASsk eB+Sn9ILUWtoFAmYEBHMoLUNBbEOe3X7Vdu86Y9o8S3kBAO7FGhINSBGXZ3iB0LczU 2WDz+JwGV0bqA== Message-ID: <136bad1c-f91c-4730-88e6-3186d9c7337e@kernel.org> Date: Wed, 15 Jan 2025 10:12:51 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH 1/2] f2fs: register inodes which is able to donate pages To: Jaegeuk Kim References: <20250113183933.1268282-1-jaegeuk@kernel.org> Content-Language: en-US From: Chao Yu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 1/15/25 01:15, Jaegeuk Kim wrote: > On 01/14, Chao Yu wrote: >> On 1/14/25 02:39, Jaegeuk Kim via Linux-f2fs-devel wrote: >>> This patch introduces an inode list to keep the page cache ranges that users >>> can donate pages together. >>> >>> #define F2FS_IOC_DONATE_RANGE _IOW(F2FS_IOCTL_MAGIC, 27, \ >>> struct f2fs_donate_range) >>> struct f2fs_donate_range { >>> __u64 start; >>> __u64 len; >>> }; >>> >>> e.g., ioctl(F2FS_IOC_DONATE_RANGE, &range); >> >> I guess we need to add documentation for all ioctls including this one, maybe >> later? :) > > Yeah, later. > >> >>> >>> Signed-off-by: Jaegeuk Kim >>> --- >>> fs/f2fs/debug.c | 3 +++ >>> fs/f2fs/f2fs.h | 9 +++++++- >>> fs/f2fs/file.c | 48 +++++++++++++++++++++++++++++++++++++++ >>> fs/f2fs/inode.c | 14 ++++++++++++ >>> fs/f2fs/super.c | 1 + >>> include/uapi/linux/f2fs.h | 7 ++++++ >>> 6 files changed, 81 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c >>> index 468828288a4a..1b099c123670 100644 >>> --- a/fs/f2fs/debug.c >>> +++ b/fs/f2fs/debug.c >>> @@ -164,6 +164,7 @@ static void update_general_status(struct f2fs_sb_info *sbi) >>> si->ndirty_imeta = get_pages(sbi, F2FS_DIRTY_IMETA); >>> si->ndirty_dirs = sbi->ndirty_inode[DIR_INODE]; >>> si->ndirty_files = sbi->ndirty_inode[FILE_INODE]; >>> + si->ndonate_files = sbi->ndirty_inode[DONATE_INODE]; >>> si->nquota_files = sbi->nquota_files; >>> si->ndirty_all = sbi->ndirty_inode[DIRTY_META]; >>> si->aw_cnt = atomic_read(&sbi->atomic_files); >>> @@ -501,6 +502,8 @@ static int stat_show(struct seq_file *s, void *v) >>> si->compr_inode, si->compr_blocks); >>> seq_printf(s, " - Swapfile Inode: %u\n", >>> si->swapfile_inode); >>> + seq_printf(s, " - Donate Inode: %d\n", >> >> %u instead of %d due to si->ndonate_files is type of unsigned int. >> >>> + si->ndonate_files); >>> seq_printf(s, " - Orphan/Append/Update Inode: %u, %u, %u\n", >>> si->orphans, si->append, si->update); >>> seq_printf(s, "\nMain area: %d segs, %d secs %d zones\n", >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 4bfe162eefd3..7ce3e3eab17a 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -850,6 +850,11 @@ struct f2fs_inode_info { >>> #endif >>> struct list_head dirty_list; /* dirty list for dirs and files */ >>> struct list_head gdirty_list; /* linked in global dirty list */ >>> + >>> + /* linked in global inode list for cache donation */ >>> + struct list_head gdonate_list; >>> + loff_t donate_start, donate_end; /* inclusive */ >>> + >>> struct task_struct *atomic_write_task; /* store atomic write task */ >>> struct extent_tree *extent_tree[NR_EXTENT_CACHES]; >>> /* cached extent_tree entry */ >>> @@ -1274,6 +1279,7 @@ enum inode_type { >>> DIR_INODE, /* for dirty dir inode */ >>> FILE_INODE, /* for dirty regular/symlink inode */ >>> DIRTY_META, /* for all dirtied inode metadata */ >>> + DONATE_INODE, /* for all inode to donate pages */ >>> NR_INODE_TYPE, >>> }; >>> @@ -3984,7 +3990,8 @@ struct f2fs_stat_info { >>> unsigned long long allocated_data_blocks; >>> int ndirty_node, ndirty_dent, ndirty_meta, ndirty_imeta; >>> int ndirty_data, ndirty_qdata; >>> - unsigned int ndirty_dirs, ndirty_files, nquota_files, ndirty_all; >>> + unsigned int ndirty_dirs, ndirty_files, ndirty_all; >>> + unsigned int nquota_files, ndonate_files; >>> int nats, dirty_nats, sits, dirty_sits; >>> int free_nids, avail_nids, alloc_nids; >>> int total_count, utilization; >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index 9980d17ef9f5..d6dea6258c2d 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -2493,6 +2493,51 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg) >>> return ret; >>> } >>> +static int f2fs_ioc_donate_range(struct file *filp, unsigned long arg) >>> +{ >>> + struct inode *inode = file_inode(filp); >>> + struct mnt_idmap *idmap = file_mnt_idmap(filp); >>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> + struct f2fs_donate_range range; >>> + int ret; >>> + >>> + if (copy_from_user(&range, (struct f2fs_donate_range __user *)arg, >>> + sizeof(range))) >>> + return -EFAULT; >> >> What about doing sanity check on donate range here? in order to avoid overflow >> during fi->donate_end calculation. >> >> F2FS_I(inode)->donate_end = range.start + range.len - 1; >> >>> + >>> + if (!inode_owner_or_capable(idmap, inode)) >>> + return -EACCES; >>> + >>> + if (!S_ISREG(inode->i_mode)) >>> + return -EINVAL; >>> + >>> + ret = mnt_want_write_file(filp); >>> + if (ret) >>> + return ret; >>> + >>> + inode_lock(inode); >>> + >>> + if (f2fs_is_atomic_file(inode)) >>> + goto out; >>> + >>> + spin_lock(&sbi->inode_lock[DONATE_INODE]); >>> + if (list_empty(&F2FS_I(inode)->gdonate_list)) { >>> + list_add_tail(&F2FS_I(inode)->gdonate_list, >>> + &sbi->inode_list[DONATE_INODE]); >>> + stat_inc_dirty_inode(sbi, DONATE_INODE); >>> + } else { >>> + list_move_tail(&F2FS_I(inode)->gdonate_list, >>> + &sbi->inode_list[DONATE_INODE]); >>> + } >>> + F2FS_I(inode)->donate_start = range.start; >>> + F2FS_I(inode)->donate_end = range.start + range.len - 1; >>> + spin_unlock(&sbi->inode_lock[DONATE_INODE]); >>> +out: >>> + inode_unlock(inode); >>> + mnt_drop_write_file(filp); >>> + return ret; >>> +} >>> + >>> static int f2fs_ioc_fitrim(struct file *filp, unsigned long arg) >>> { >>> struct inode *inode = file_inode(filp); >>> @@ -4522,6 +4567,8 @@ static long __f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) >>> return -EOPNOTSUPP; >>> case F2FS_IOC_SHUTDOWN: >>> return f2fs_ioc_shutdown(filp, arg); >>> + case F2FS_IOC_DONATE_RANGE: >>> + return f2fs_ioc_donate_range(filp, arg); >>> case FITRIM: >>> return f2fs_ioc_fitrim(filp, arg); >>> case FS_IOC_SET_ENCRYPTION_POLICY: >>> @@ -5273,6 +5320,7 @@ long f2fs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) >>> case F2FS_IOC_RELEASE_VOLATILE_WRITE: >>> case F2FS_IOC_ABORT_ATOMIC_WRITE: >>> case F2FS_IOC_SHUTDOWN: >>> + case F2FS_IOC_DONATE_RANGE: >>> case FITRIM: >>> case FS_IOC_SET_ENCRYPTION_POLICY: >>> case FS_IOC_GET_ENCRYPTION_PWSALT: >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>> index 7de33da8b3ea..e38dc5fe2f2e 100644 >>> --- a/fs/f2fs/inode.c >>> +++ b/fs/f2fs/inode.c >>> @@ -804,6 +804,19 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >>> return 0; >>> } >>> +static void f2fs_remove_donate_inode(struct inode *inode) >>> +{ >>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> + >>> + if (list_empty(&F2FS_I(inode)->gdonate_list)) >> >> It will be more safe to access gdonate_list w/ inode_lock[DONATE_INODE]? > > It's unnecessary as this is called from evict_inode. I just concerned about the case fi->gdonate_list's prev and next pointer can be updated in race condition due to insertion or deletion of its adjacent entry. No risk now as I checked. :) Thanks, > >> >> Thanks, >> >>> + return; >>> + >>> + spin_lock(&sbi->inode_lock[DONATE_INODE]); >>> + list_del_init(&F2FS_I(inode)->gdonate_list); >>> + stat_dec_dirty_inode(sbi, DONATE_INODE); >>> + spin_unlock(&sbi->inode_lock[DONATE_INODE]); >>> +} >>> + >>> /* >>> * Called at the last iput() if i_nlink is zero >>> */ >>> @@ -838,6 +851,7 @@ void f2fs_evict_inode(struct inode *inode) >>> f2fs_bug_on(sbi, get_dirty_pages(inode)); >>> f2fs_remove_dirty_inode(inode); >>> + f2fs_remove_donate_inode(inode); >>> if (!IS_DEVICE_ALIASING(inode)) >>> f2fs_destroy_extent_tree(inode); >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index fc7d463dee15..ef639a6d82e5 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -1441,6 +1441,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb) >>> spin_lock_init(&fi->i_size_lock); >>> INIT_LIST_HEAD(&fi->dirty_list); >>> INIT_LIST_HEAD(&fi->gdirty_list); >>> + INIT_LIST_HEAD(&fi->gdonate_list); >>> init_f2fs_rwsem(&fi->i_gc_rwsem[READ]); >>> init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]); >>> init_f2fs_rwsem(&fi->i_xattr_sem); >>> diff --git a/include/uapi/linux/f2fs.h b/include/uapi/linux/f2fs.h >>> index f7aaf8d23e20..cd38a7c166e6 100644 >>> --- a/include/uapi/linux/f2fs.h >>> +++ b/include/uapi/linux/f2fs.h >>> @@ -44,6 +44,8 @@ >>> #define F2FS_IOC_COMPRESS_FILE _IO(F2FS_IOCTL_MAGIC, 24) >>> #define F2FS_IOC_START_ATOMIC_REPLACE _IO(F2FS_IOCTL_MAGIC, 25) >>> #define F2FS_IOC_GET_DEV_ALIAS_FILE _IOR(F2FS_IOCTL_MAGIC, 26, __u32) >>> +#define F2FS_IOC_DONATE_RANGE _IOW(F2FS_IOCTL_MAGIC, 27, \ >>> + struct f2fs_donate_range) >>> /* >>> * should be same as XFS_IOC_GOINGDOWN. >>> @@ -97,4 +99,9 @@ struct f2fs_comp_option { >>> __u8 log_cluster_size; >>> }; >>> +struct f2fs_donate_range { >>> + __u64 start; >>> + __u64 len; >>> +}; >>> + >>> #endif /* _UAPI_LINUX_F2FS_H */