diff mbox series

[f2fs-dev,RFC] f2fs: fix to use spinlock to avoid page.private update race

Message ID 20230410093912.2184557-1-chao@kernel.org (mailing list archive)
State New
Headers show
Series [f2fs-dev,RFC] f2fs: fix to use spinlock to avoid page.private update race | expand

Commit Message

Chao Yu April 10, 2023, 9:39 a.m. UTC 
There may be subtle race condition, make PagePrivate and page_private
being inconsistent, result in decreasing page count incorrectly,
introduce a per-inode spinlock to avoid such condition.

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/f2fs.h  | 19 ++++++++++++++++++-
 fs/f2fs/super.c |  2 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

Comments

Jaegeuk Kim April 10, 2023, 11:25 p.m. UTC | #1 
On 04/10, Chao Yu wrote:
> There may be subtle race condition, make PagePrivate and page_private
> being inconsistent, result in decreasing page count incorrectly,
> introduce a per-inode spinlock to avoid such condition.

No...what have you found? The set/clear.. were supposed to be done in page_lock,
and checking the flag should not corrupt any memory.

> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/f2fs.h  | 19 ++++++++++++++++++-
>  fs/f2fs/super.c |  2 ++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index c378aedcadea..6b31bef5853e 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -856,6 +856,8 @@ struct f2fs_inode_info {
>  
>  	unsigned int atomic_write_cnt;
>  	loff_t original_i_size;		/* original i_size before atomic write */
> +
> +	spinlock_t private_lock;	/* protect page->private */
>  };
>  
>  static inline void get_read_extent_info(struct extent_info *ext,
> @@ -1413,21 +1415,28 @@ static inline bool page_private_##name(struct page *page) \
>  		test_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
>  }
>  
> +static inline struct f2fs_inode_info *F2FS_I(struct inode *inode);
>  #define PAGE_PRIVATE_SET_FUNC(name, flagname) \
>  static inline void set_page_private_##name(struct page *page) \
>  { \
> +	unsigned long flags; \
> +	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags); \
>  	if (!PagePrivate(page)) \
>  		attach_page_private(page, (void *)page->private); \
>  	set_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page)); \
>  	set_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
> +	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags); \
>  }
>  
>  #define PAGE_PRIVATE_CLEAR_FUNC(name, flagname) \
>  static inline void clear_page_private_##name(struct page *page) \
>  { \
> +	unsigned long flags; \
> +	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags); \
>  	clear_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
> -	if (page_private(page) == BIT(PAGE_PRIVATE_NOT_POINTER)) \
> +	if (page_private(page) == (BIT(PAGE_PRIVATE_NOT_POINTER))) \
>  		detach_page_private(page); \
> +	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags); \
>  }
>  
>  PAGE_PRIVATE_GET_FUNC(nonpointer, NOT_POINTER);
> @@ -1456,17 +1465,25 @@ static inline unsigned long get_page_private_data(struct page *page)
>  
>  static inline void set_page_private_data(struct page *page, unsigned long data)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags);
>  	if (!PagePrivate(page))
>  		attach_page_private(page, 0);
>  	set_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page));
>  	page_private(page) |= data << PAGE_PRIVATE_MAX;
> +	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags);
>  }
>  
>  static inline void clear_page_private_data(struct page *page)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags);
>  	page_private(page) &= GENMASK(PAGE_PRIVATE_MAX - 1, 0);
>  	if (page_private(page) == BIT(PAGE_PRIVATE_NOT_POINTER))
>  		detach_page_private(page);
> +	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags);
>  }
>  
>  /* For compression */
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index a1b570a5e50f..555424dd85fd 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1419,6 +1419,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>  	init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
>  	init_f2fs_rwsem(&fi->i_xattr_sem);
>  
> +	spin_lock_init(&fi->private_lock);
> +
>  	/* Will be used by directory only */
>  	fi->i_dir_level = F2FS_SB(sb)->dir_level;
>  
> -- 
> 2.25.1
diff mbox series

Patch

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c378aedcadea..6b31bef5853e 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -856,6 +856,8 @@  struct f2fs_inode_info {
 
 	unsigned int atomic_write_cnt;
 	loff_t original_i_size;		/* original i_size before atomic write */
+
+	spinlock_t private_lock;	/* protect page->private */
 };
 
 static inline void get_read_extent_info(struct extent_info *ext,
@@ -1413,21 +1415,28 @@  static inline bool page_private_##name(struct page *page) \
 		test_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
 }
 
+static inline struct f2fs_inode_info *F2FS_I(struct inode *inode);
 #define PAGE_PRIVATE_SET_FUNC(name, flagname) \
 static inline void set_page_private_##name(struct page *page) \
 { \
+	unsigned long flags; \
+	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags); \
 	if (!PagePrivate(page)) \
 		attach_page_private(page, (void *)page->private); \
 	set_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page)); \
 	set_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
+	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags); \
 }
 
 #define PAGE_PRIVATE_CLEAR_FUNC(name, flagname) \
 static inline void clear_page_private_##name(struct page *page) \
 { \
+	unsigned long flags; \
+	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags); \
 	clear_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
-	if (page_private(page) == BIT(PAGE_PRIVATE_NOT_POINTER)) \
+	if (page_private(page) == (BIT(PAGE_PRIVATE_NOT_POINTER))) \
 		detach_page_private(page); \
+	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags); \
 }
 
 PAGE_PRIVATE_GET_FUNC(nonpointer, NOT_POINTER);
@@ -1456,17 +1465,25 @@  static inline unsigned long get_page_private_data(struct page *page)
 
 static inline void set_page_private_data(struct page *page, unsigned long data)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags);
 	if (!PagePrivate(page))
 		attach_page_private(page, 0);
 	set_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page));
 	page_private(page) |= data << PAGE_PRIVATE_MAX;
+	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags);
 }
 
 static inline void clear_page_private_data(struct page *page)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&F2FS_I(page->mapping->host)->private_lock, flags);
 	page_private(page) &= GENMASK(PAGE_PRIVATE_MAX - 1, 0);
 	if (page_private(page) == BIT(PAGE_PRIVATE_NOT_POINTER))
 		detach_page_private(page);
+	spin_unlock_irqrestore(&F2FS_I(page->mapping->host)->private_lock, flags);
 }
 
 /* For compression */
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index a1b570a5e50f..555424dd85fd 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1419,6 +1419,8 @@  static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]);
 	init_f2fs_rwsem(&fi->i_xattr_sem);
 
+	spin_lock_init(&fi->private_lock);
+
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;