All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-10 18:32 ` Daeho Jeong
  0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2022-06-10 18:32 UTC (permalink / raw)
  To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

From: Daeho Jeong <daehojeong@google.com>

Made iostat related locks safe to be called from irq context again.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
index be599f31d3c4..d84c5f6cc09d 100644
--- a/fs/f2fs/iostat.c
+++ b/fs/f2fs/iostat.c
@@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
 	unsigned int cnt;
 	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
 	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
+	unsigned long flags;
 
-	spin_lock_bh(&sbi->iostat_lat_lock);
+	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
 		for (io = 0; io < NR_PAGE_TYPE; io++) {
 			cnt = io_lat->bio_cnt[idx][io];
@@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
 			io_lat->bio_cnt[idx][io] = 0;
 		}
 	}
-	spin_unlock_bh(&sbi->iostat_lat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
 
 	trace_f2fs_iostat_latency(sbi, iostat_lat);
 }
@@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
 {
 	unsigned long long iostat_diff[NR_IO_TYPE];
 	int i;
+	unsigned long flags;
 
 	if (time_is_after_jiffies(sbi->iostat_next_period))
 		return;
 
 	/* Need double check under the lock */
-	spin_lock_bh(&sbi->iostat_lock);
+	spin_lock_irqsave(&sbi->iostat_lock, flags);
 	if (time_is_after_jiffies(sbi->iostat_next_period)) {
-		spin_unlock_bh(&sbi->iostat_lock);
+		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
 		return;
 	}
 	sbi->iostat_next_period = jiffies +
@@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
 				sbi->prev_rw_iostat[i];
 		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
 	}
-	spin_unlock_bh(&sbi->iostat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
 
 	trace_f2fs_iostat(sbi, iostat_diff);
 
@@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
 	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
 	int i;
 
-	spin_lock_bh(&sbi->iostat_lock);
+	spin_lock_irq(&sbi->iostat_lock);
 	for (i = 0; i < NR_IO_TYPE; i++) {
 		sbi->rw_iostat[i] = 0;
 		sbi->prev_rw_iostat[i] = 0;
 	}
-	spin_unlock_bh(&sbi->iostat_lock);
+	spin_unlock_irq(&sbi->iostat_lock);
 
-	spin_lock_bh(&sbi->iostat_lat_lock);
+	spin_lock_irq(&sbi->iostat_lat_lock);
 	memset(io_lat, 0, sizeof(struct iostat_lat_info));
-	spin_unlock_bh(&sbi->iostat_lat_lock);
+	spin_unlock_irq(&sbi->iostat_lat_lock);
 }
 
 void f2fs_update_iostat(struct f2fs_sb_info *sbi,
 			enum iostat_type type, unsigned long long io_bytes)
 {
+	unsigned long flags;
+
 	if (!sbi->iostat_enable)
 		return;
 
-	spin_lock_bh(&sbi->iostat_lock);
+	spin_lock_irqsave(&sbi->iostat_lock, flags);
 	sbi->rw_iostat[type] += io_bytes;
 
 	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
@@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
 	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
 		sbi->rw_iostat[APP_READ_IO] += io_bytes;
 
-	spin_unlock_bh(&sbi->iostat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
 
 	f2fs_record_iostat(sbi);
 }
@@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
 	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
 	int idx;
+	unsigned long flags;
 
 	if (!sbi->iostat_enable)
 		return;
@@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 			idx = WRITE_ASYNC_IO;
 	}
 
-	spin_lock_bh(&sbi->iostat_lat_lock);
+	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 	io_lat->sum_lat[idx][iotype] += ts_diff;
 	io_lat->bio_cnt[idx][iotype]++;
 	if (ts_diff > io_lat->peak_lat[idx][iotype])
 		io_lat->peak_lat[idx][iotype] = ts_diff;
-	spin_unlock_bh(&sbi->iostat_lat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
 }
 
 void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
-- 
2.36.1.476.g0c4daa206d-goog


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-10 18:32 ` Daeho Jeong
  0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2022-06-10 18:32 UTC (permalink / raw)
  To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

From: Daeho Jeong <daehojeong@google.com>

Made iostat related locks safe to be called from irq context again.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
index be599f31d3c4..d84c5f6cc09d 100644
--- a/fs/f2fs/iostat.c
+++ b/fs/f2fs/iostat.c
@@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
 	unsigned int cnt;
 	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
 	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
+	unsigned long flags;
 
-	spin_lock_bh(&sbi->iostat_lat_lock);
+	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
 		for (io = 0; io < NR_PAGE_TYPE; io++) {
 			cnt = io_lat->bio_cnt[idx][io];
@@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
 			io_lat->bio_cnt[idx][io] = 0;
 		}
 	}
-	spin_unlock_bh(&sbi->iostat_lat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
 
 	trace_f2fs_iostat_latency(sbi, iostat_lat);
 }
@@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
 {
 	unsigned long long iostat_diff[NR_IO_TYPE];
 	int i;
+	unsigned long flags;
 
 	if (time_is_after_jiffies(sbi->iostat_next_period))
 		return;
 
 	/* Need double check under the lock */
-	spin_lock_bh(&sbi->iostat_lock);
+	spin_lock_irqsave(&sbi->iostat_lock, flags);
 	if (time_is_after_jiffies(sbi->iostat_next_period)) {
-		spin_unlock_bh(&sbi->iostat_lock);
+		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
 		return;
 	}
 	sbi->iostat_next_period = jiffies +
@@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
 				sbi->prev_rw_iostat[i];
 		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
 	}
-	spin_unlock_bh(&sbi->iostat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
 
 	trace_f2fs_iostat(sbi, iostat_diff);
 
@@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
 	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
 	int i;
 
-	spin_lock_bh(&sbi->iostat_lock);
+	spin_lock_irq(&sbi->iostat_lock);
 	for (i = 0; i < NR_IO_TYPE; i++) {
 		sbi->rw_iostat[i] = 0;
 		sbi->prev_rw_iostat[i] = 0;
 	}
-	spin_unlock_bh(&sbi->iostat_lock);
+	spin_unlock_irq(&sbi->iostat_lock);
 
-	spin_lock_bh(&sbi->iostat_lat_lock);
+	spin_lock_irq(&sbi->iostat_lat_lock);
 	memset(io_lat, 0, sizeof(struct iostat_lat_info));
-	spin_unlock_bh(&sbi->iostat_lat_lock);
+	spin_unlock_irq(&sbi->iostat_lat_lock);
 }
 
 void f2fs_update_iostat(struct f2fs_sb_info *sbi,
 			enum iostat_type type, unsigned long long io_bytes)
 {
+	unsigned long flags;
+
 	if (!sbi->iostat_enable)
 		return;
 
-	spin_lock_bh(&sbi->iostat_lock);
+	spin_lock_irqsave(&sbi->iostat_lock, flags);
 	sbi->rw_iostat[type] += io_bytes;
 
 	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
@@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
 	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
 		sbi->rw_iostat[APP_READ_IO] += io_bytes;
 
-	spin_unlock_bh(&sbi->iostat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
 
 	f2fs_record_iostat(sbi);
 }
@@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
 	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
 	int idx;
+	unsigned long flags;
 
 	if (!sbi->iostat_enable)
 		return;
@@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 			idx = WRITE_ASYNC_IO;
 	}
 
-	spin_lock_bh(&sbi->iostat_lat_lock);
+	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 	io_lat->sum_lat[idx][iotype] += ts_diff;
 	io_lat->bio_cnt[idx][iotype]++;
 	if (ts_diff > io_lat->peak_lat[idx][iotype])
 		io_lat->peak_lat[idx][iotype] = ts_diff;
-	spin_unlock_bh(&sbi->iostat_lat_lock);
+	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
 }
 
 void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
-- 
2.36.1.476.g0c4daa206d-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-10 18:32 ` [f2fs-dev] " Daeho Jeong
@ 2022-06-15  7:54   ` Chao Yu
  -1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-15  7:54 UTC (permalink / raw)
  To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

On 2022/6/11 2:32, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Made iostat related locks safe to be called from irq context again.
> 

Will be better to add a 'Fixes' line?

Thanks,

> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> index be599f31d3c4..d84c5f6cc09d 100644
> --- a/fs/f2fs/iostat.c
> +++ b/fs/f2fs/iostat.c
> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>   	unsigned int cnt;
>   	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> +	unsigned long flags;
>   
> -	spin_lock_bh(&sbi->iostat_lat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>   		for (io = 0; io < NR_PAGE_TYPE; io++) {
>   			cnt = io_lat->bio_cnt[idx][io];
> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>   			io_lat->bio_cnt[idx][io] = 0;
>   		}
>   	}
> -	spin_unlock_bh(&sbi->iostat_lat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>   
>   	trace_f2fs_iostat_latency(sbi, iostat_lat);
>   }
> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>   {
>   	unsigned long long iostat_diff[NR_IO_TYPE];
>   	int i;
> +	unsigned long flags;
>   
>   	if (time_is_after_jiffies(sbi->iostat_next_period))
>   		return;
>   
>   	/* Need double check under the lock */
> -	spin_lock_bh(&sbi->iostat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>   	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> -		spin_unlock_bh(&sbi->iostat_lock);
> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>   		return;
>   	}
>   	sbi->iostat_next_period = jiffies +
> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>   				sbi->prev_rw_iostat[i];
>   		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>   	}
> -	spin_unlock_bh(&sbi->iostat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>   
>   	trace_f2fs_iostat(sbi, iostat_diff);
>   
> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>   	int i;
>   
> -	spin_lock_bh(&sbi->iostat_lock);
> +	spin_lock_irq(&sbi->iostat_lock);
>   	for (i = 0; i < NR_IO_TYPE; i++) {
>   		sbi->rw_iostat[i] = 0;
>   		sbi->prev_rw_iostat[i] = 0;
>   	}
> -	spin_unlock_bh(&sbi->iostat_lock);
> +	spin_unlock_irq(&sbi->iostat_lock);
>   
> -	spin_lock_bh(&sbi->iostat_lat_lock);
> +	spin_lock_irq(&sbi->iostat_lat_lock);
>   	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> -	spin_unlock_bh(&sbi->iostat_lat_lock);
> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>   }
>   
>   void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>   			enum iostat_type type, unsigned long long io_bytes)
>   {
> +	unsigned long flags;
> +
>   	if (!sbi->iostat_enable)
>   		return;
>   
> -	spin_lock_bh(&sbi->iostat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>   	sbi->rw_iostat[type] += io_bytes;
>   
>   	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>   	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>   		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>   
> -	spin_unlock_bh(&sbi->iostat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>   
>   	f2fs_record_iostat(sbi);
>   }
> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>   	int idx;
> +	unsigned long flags;
>   
>   	if (!sbi->iostat_enable)
>   		return;
> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   			idx = WRITE_ASYNC_IO;
>   	}
>   
> -	spin_lock_bh(&sbi->iostat_lat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   	io_lat->sum_lat[idx][iotype] += ts_diff;
>   	io_lat->bio_cnt[idx][iotype]++;
>   	if (ts_diff > io_lat->peak_lat[idx][iotype])
>   		io_lat->peak_lat[idx][iotype] = ts_diff;
> -	spin_unlock_bh(&sbi->iostat_lat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>   }
>   
>   void iostat_update_and_unbind_ctx(struct bio *bio, int rw)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-15  7:54   ` Chao Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-15  7:54 UTC (permalink / raw)
  To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

On 2022/6/11 2:32, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Made iostat related locks safe to be called from irq context again.
> 

Will be better to add a 'Fixes' line?

Thanks,

> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> index be599f31d3c4..d84c5f6cc09d 100644
> --- a/fs/f2fs/iostat.c
> +++ b/fs/f2fs/iostat.c
> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>   	unsigned int cnt;
>   	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> +	unsigned long flags;
>   
> -	spin_lock_bh(&sbi->iostat_lat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>   		for (io = 0; io < NR_PAGE_TYPE; io++) {
>   			cnt = io_lat->bio_cnt[idx][io];
> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>   			io_lat->bio_cnt[idx][io] = 0;
>   		}
>   	}
> -	spin_unlock_bh(&sbi->iostat_lat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>   
>   	trace_f2fs_iostat_latency(sbi, iostat_lat);
>   }
> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>   {
>   	unsigned long long iostat_diff[NR_IO_TYPE];
>   	int i;
> +	unsigned long flags;
>   
>   	if (time_is_after_jiffies(sbi->iostat_next_period))
>   		return;
>   
>   	/* Need double check under the lock */
> -	spin_lock_bh(&sbi->iostat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>   	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> -		spin_unlock_bh(&sbi->iostat_lock);
> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>   		return;
>   	}
>   	sbi->iostat_next_period = jiffies +
> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>   				sbi->prev_rw_iostat[i];
>   		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>   	}
> -	spin_unlock_bh(&sbi->iostat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>   
>   	trace_f2fs_iostat(sbi, iostat_diff);
>   
> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>   	int i;
>   
> -	spin_lock_bh(&sbi->iostat_lock);
> +	spin_lock_irq(&sbi->iostat_lock);
>   	for (i = 0; i < NR_IO_TYPE; i++) {
>   		sbi->rw_iostat[i] = 0;
>   		sbi->prev_rw_iostat[i] = 0;
>   	}
> -	spin_unlock_bh(&sbi->iostat_lock);
> +	spin_unlock_irq(&sbi->iostat_lock);
>   
> -	spin_lock_bh(&sbi->iostat_lat_lock);
> +	spin_lock_irq(&sbi->iostat_lat_lock);
>   	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> -	spin_unlock_bh(&sbi->iostat_lat_lock);
> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>   }
>   
>   void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>   			enum iostat_type type, unsigned long long io_bytes)
>   {
> +	unsigned long flags;
> +
>   	if (!sbi->iostat_enable)
>   		return;
>   
> -	spin_lock_bh(&sbi->iostat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>   	sbi->rw_iostat[type] += io_bytes;
>   
>   	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>   	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>   		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>   
> -	spin_unlock_bh(&sbi->iostat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>   
>   	f2fs_record_iostat(sbi);
>   }
> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>   	int idx;
> +	unsigned long flags;
>   
>   	if (!sbi->iostat_enable)
>   		return;
> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   			idx = WRITE_ASYNC_IO;
>   	}
>   
> -	spin_lock_bh(&sbi->iostat_lat_lock);
> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   	io_lat->sum_lat[idx][iotype] += ts_diff;
>   	io_lat->bio_cnt[idx][iotype]++;
>   	if (ts_diff > io_lat->peak_lat[idx][iotype])
>   		io_lat->peak_lat[idx][iotype] = ts_diff;
> -	spin_unlock_bh(&sbi->iostat_lat_lock);
> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>   }
>   
>   void iostat_update_and_unbind_ctx(struct bio *bio, int rw)


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-15  7:54   ` Chao Yu
@ 2022-06-15 16:54     ` Jaegeuk Kim
  -1 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2022-06-15 16:54 UTC (permalink / raw)
  To: Chao Yu
  Cc: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On 06/15, Chao Yu wrote:
> On 2022/6/11 2:32, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> > 
> > Made iostat related locks safe to be called from irq context again.
> > 
> 
> Will be better to add a 'Fixes' line?

Added some tags. Thanks,

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4


> 
> Thanks,
> 
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >   fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
> >   1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> > index be599f31d3c4..d84c5f6cc09d 100644
> > --- a/fs/f2fs/iostat.c
> > +++ b/fs/f2fs/iostat.c
> > @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> >   	unsigned int cnt;
> >   	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
> >   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > +	unsigned long flags;
> > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> >   	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
> >   		for (io = 0; io < NR_PAGE_TYPE; io++) {
> >   			cnt = io_lat->bio_cnt[idx][io];
> > @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> >   			io_lat->bio_cnt[idx][io] = 0;
> >   		}
> >   	}
> > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> >   	trace_f2fs_iostat_latency(sbi, iostat_lat);
> >   }
> > @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> >   {
> >   	unsigned long long iostat_diff[NR_IO_TYPE];
> >   	int i;
> > +	unsigned long flags;
> >   	if (time_is_after_jiffies(sbi->iostat_next_period))
> >   		return;
> >   	/* Need double check under the lock */
> > -	spin_lock_bh(&sbi->iostat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> >   	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> > -		spin_unlock_bh(&sbi->iostat_lock);
> > +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> >   		return;
> >   	}
> >   	sbi->iostat_next_period = jiffies +
> > @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> >   				sbi->prev_rw_iostat[i];
> >   		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
> >   	}
> > -	spin_unlock_bh(&sbi->iostat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> >   	trace_f2fs_iostat(sbi, iostat_diff);
> > @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> >   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> >   	int i;
> > -	spin_lock_bh(&sbi->iostat_lock);
> > +	spin_lock_irq(&sbi->iostat_lock);
> >   	for (i = 0; i < NR_IO_TYPE; i++) {
> >   		sbi->rw_iostat[i] = 0;
> >   		sbi->prev_rw_iostat[i] = 0;
> >   	}
> > -	spin_unlock_bh(&sbi->iostat_lock);
> > +	spin_unlock_irq(&sbi->iostat_lock);
> > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > +	spin_lock_irq(&sbi->iostat_lat_lock);
> >   	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > +	spin_unlock_irq(&sbi->iostat_lat_lock);
> >   }
> >   void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> >   			enum iostat_type type, unsigned long long io_bytes)
> >   {
> > +	unsigned long flags;
> > +
> >   	if (!sbi->iostat_enable)
> >   		return;
> > -	spin_lock_bh(&sbi->iostat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> >   	sbi->rw_iostat[type] += io_bytes;
> >   	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> > @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> >   	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
> >   		sbi->rw_iostat[APP_READ_IO] += io_bytes;
> > -	spin_unlock_bh(&sbi->iostat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> >   	f2fs_record_iostat(sbi);
> >   }
> > @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> >   	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
> >   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> >   	int idx;
> > +	unsigned long flags;
> >   	if (!sbi->iostat_enable)
> >   		return;
> > @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> >   			idx = WRITE_ASYNC_IO;
> >   	}
> > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> >   	io_lat->sum_lat[idx][iotype] += ts_diff;
> >   	io_lat->bio_cnt[idx][iotype]++;
> >   	if (ts_diff > io_lat->peak_lat[idx][iotype])
> >   		io_lat->peak_lat[idx][iotype] = ts_diff;
> > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> >   }
> >   void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-15 16:54     ` Jaegeuk Kim
  0 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2022-06-15 16:54 UTC (permalink / raw)
  To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 06/15, Chao Yu wrote:
> On 2022/6/11 2:32, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> > 
> > Made iostat related locks safe to be called from irq context again.
> > 
> 
> Will be better to add a 'Fixes' line?

Added some tags. Thanks,

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4


> 
> Thanks,
> 
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >   fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
> >   1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> > index be599f31d3c4..d84c5f6cc09d 100644
> > --- a/fs/f2fs/iostat.c
> > +++ b/fs/f2fs/iostat.c
> > @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> >   	unsigned int cnt;
> >   	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
> >   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > +	unsigned long flags;
> > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> >   	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
> >   		for (io = 0; io < NR_PAGE_TYPE; io++) {
> >   			cnt = io_lat->bio_cnt[idx][io];
> > @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> >   			io_lat->bio_cnt[idx][io] = 0;
> >   		}
> >   	}
> > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> >   	trace_f2fs_iostat_latency(sbi, iostat_lat);
> >   }
> > @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> >   {
> >   	unsigned long long iostat_diff[NR_IO_TYPE];
> >   	int i;
> > +	unsigned long flags;
> >   	if (time_is_after_jiffies(sbi->iostat_next_period))
> >   		return;
> >   	/* Need double check under the lock */
> > -	spin_lock_bh(&sbi->iostat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> >   	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> > -		spin_unlock_bh(&sbi->iostat_lock);
> > +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> >   		return;
> >   	}
> >   	sbi->iostat_next_period = jiffies +
> > @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> >   				sbi->prev_rw_iostat[i];
> >   		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
> >   	}
> > -	spin_unlock_bh(&sbi->iostat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> >   	trace_f2fs_iostat(sbi, iostat_diff);
> > @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> >   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> >   	int i;
> > -	spin_lock_bh(&sbi->iostat_lock);
> > +	spin_lock_irq(&sbi->iostat_lock);
> >   	for (i = 0; i < NR_IO_TYPE; i++) {
> >   		sbi->rw_iostat[i] = 0;
> >   		sbi->prev_rw_iostat[i] = 0;
> >   	}
> > -	spin_unlock_bh(&sbi->iostat_lock);
> > +	spin_unlock_irq(&sbi->iostat_lock);
> > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > +	spin_lock_irq(&sbi->iostat_lat_lock);
> >   	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > +	spin_unlock_irq(&sbi->iostat_lat_lock);
> >   }
> >   void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> >   			enum iostat_type type, unsigned long long io_bytes)
> >   {
> > +	unsigned long flags;
> > +
> >   	if (!sbi->iostat_enable)
> >   		return;
> > -	spin_lock_bh(&sbi->iostat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> >   	sbi->rw_iostat[type] += io_bytes;
> >   	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> > @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> >   	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
> >   		sbi->rw_iostat[APP_READ_IO] += io_bytes;
> > -	spin_unlock_bh(&sbi->iostat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> >   	f2fs_record_iostat(sbi);
> >   }
> > @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> >   	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
> >   	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> >   	int idx;
> > +	unsigned long flags;
> >   	if (!sbi->iostat_enable)
> >   		return;
> > @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> >   			idx = WRITE_ASYNC_IO;
> >   	}
> > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> >   	io_lat->sum_lat[idx][iotype] += ts_diff;
> >   	io_lat->bio_cnt[idx][iotype]++;
> >   	if (ts_diff > io_lat->peak_lat[idx][iotype])
> >   		io_lat->peak_lat[idx][iotype] = ts_diff;
> > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> >   }
> >   void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-15 16:54     ` Jaegeuk Kim
@ 2022-06-16  2:11       ` Chao Yu
  -1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-16  2:11 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On 2022/6/16 0:54, Jaegeuk Kim wrote:
> On 06/15, Chao Yu wrote:
>> On 2022/6/11 2:32, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> Made iostat related locks safe to be called from irq context again.
>>>
>>
>> Will be better to add a 'Fixes' line?
> 
> Added some tags. Thanks,
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4

It looks there are several patches not in mailing list?

Thanks,

> 
> 
>>
>> Thanks,
>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>>    fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>>>    1 file changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
>>> index be599f31d3c4..d84c5f6cc09d 100644
>>> --- a/fs/f2fs/iostat.c
>>> +++ b/fs/f2fs/iostat.c
>>> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    	unsigned int cnt;
>>>    	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>> +	unsigned long flags;
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>>>    		for (io = 0; io < NR_PAGE_TYPE; io++) {
>>>    			cnt = io_lat->bio_cnt[idx][io];
>>> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    			io_lat->bio_cnt[idx][io] = 0;
>>>    		}
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    	trace_f2fs_iostat_latency(sbi, iostat_lat);
>>>    }
>>> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    {
>>>    	unsigned long long iostat_diff[NR_IO_TYPE];
>>>    	int i;
>>> +	unsigned long flags;
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period))
>>>    		return;
>>>    	/* Need double check under the lock */
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period)) {
>>> -		spin_unlock_bh(&sbi->iostat_lock);
>>> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    		return;
>>>    	}
>>>    	sbi->iostat_next_period = jiffies +
>>> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    				sbi->prev_rw_iostat[i];
>>>    		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	trace_f2fs_iostat(sbi, iostat_diff);
>>> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int i;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lock);
>>>    	for (i = 0; i < NR_IO_TYPE; i++) {
>>>    		sbi->rw_iostat[i] = 0;
>>>    		sbi->prev_rw_iostat[i] = 0;
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lock);
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lat_lock);
>>>    	memset(io_lat, 0, sizeof(struct iostat_lat_info));
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>>>    }
>>>    void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    			enum iostat_type type, unsigned long long io_bytes)
>>>    {
>>> +	unsigned long flags;
>>> +
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	sbi->rw_iostat[type] += io_bytes;
>>>    	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
>>> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>>>    		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	f2fs_record_iostat(sbi);
>>>    }
>>> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int idx;
>>> +	unsigned long flags;
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    			idx = WRITE_ASYNC_IO;
>>>    	}
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	io_lat->sum_lat[idx][iotype] += ts_diff;
>>>    	io_lat->bio_cnt[idx][iotype]++;
>>>    	if (ts_diff > io_lat->peak_lat[idx][iotype])
>>>    		io_lat->peak_lat[idx][iotype] = ts_diff;
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    }
>>>    void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-16  2:11       ` Chao Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-16  2:11 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 2022/6/16 0:54, Jaegeuk Kim wrote:
> On 06/15, Chao Yu wrote:
>> On 2022/6/11 2:32, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> Made iostat related locks safe to be called from irq context again.
>>>
>>
>> Will be better to add a 'Fixes' line?
> 
> Added some tags. Thanks,
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4

It looks there are several patches not in mailing list?

Thanks,

> 
> 
>>
>> Thanks,
>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>>    fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>>>    1 file changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
>>> index be599f31d3c4..d84c5f6cc09d 100644
>>> --- a/fs/f2fs/iostat.c
>>> +++ b/fs/f2fs/iostat.c
>>> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    	unsigned int cnt;
>>>    	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>> +	unsigned long flags;
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>>>    		for (io = 0; io < NR_PAGE_TYPE; io++) {
>>>    			cnt = io_lat->bio_cnt[idx][io];
>>> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    			io_lat->bio_cnt[idx][io] = 0;
>>>    		}
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    	trace_f2fs_iostat_latency(sbi, iostat_lat);
>>>    }
>>> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    {
>>>    	unsigned long long iostat_diff[NR_IO_TYPE];
>>>    	int i;
>>> +	unsigned long flags;
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period))
>>>    		return;
>>>    	/* Need double check under the lock */
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period)) {
>>> -		spin_unlock_bh(&sbi->iostat_lock);
>>> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    		return;
>>>    	}
>>>    	sbi->iostat_next_period = jiffies +
>>> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    				sbi->prev_rw_iostat[i];
>>>    		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	trace_f2fs_iostat(sbi, iostat_diff);
>>> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int i;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lock);
>>>    	for (i = 0; i < NR_IO_TYPE; i++) {
>>>    		sbi->rw_iostat[i] = 0;
>>>    		sbi->prev_rw_iostat[i] = 0;
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lock);
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lat_lock);
>>>    	memset(io_lat, 0, sizeof(struct iostat_lat_info));
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>>>    }
>>>    void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    			enum iostat_type type, unsigned long long io_bytes)
>>>    {
>>> +	unsigned long flags;
>>> +
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	sbi->rw_iostat[type] += io_bytes;
>>>    	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
>>> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>>>    		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	f2fs_record_iostat(sbi);
>>>    }
>>> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int idx;
>>> +	unsigned long flags;
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    			idx = WRITE_ASYNC_IO;
>>>    	}
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	io_lat->sum_lat[idx][iotype] += ts_diff;
>>>    	io_lat->bio_cnt[idx][iotype]++;
>>>    	if (ts_diff > io_lat->peak_lat[idx][iotype])
>>>    		io_lat->peak_lat[idx][iotype] = ts_diff;
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    }
>>>    void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-16  2:11       ` Chao Yu
@ 2022-06-16 16:22         ` Jaegeuk Kim
  -1 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2022-06-16 16:22 UTC (permalink / raw)
  To: Chao Yu
  Cc: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On 06/16, Chao Yu wrote:
> On 2022/6/16 0:54, Jaegeuk Kim wrote:
> > On 06/15, Chao Yu wrote:
> > > On 2022/6/11 2:32, Daeho Jeong wrote:
> > > > From: Daeho Jeong <daehojeong@google.com>
> > > > 
> > > > Made iostat related locks safe to be called from irq context again.
> > > > 
> > > 
> > > Will be better to add a 'Fixes' line?
> > 
> > Added some tags. Thanks,
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4
> 
> It looks there are several patches not in mailing list?
> 

Which one doe you mean?

> Thanks,
> 
> > 
> > 
> > > 
> > > Thanks,
> > > 
> > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > ---
> > > >    fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
> > > >    1 file changed, 18 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> > > > index be599f31d3c4..d84c5f6cc09d 100644
> > > > --- a/fs/f2fs/iostat.c
> > > > +++ b/fs/f2fs/iostat.c
> > > > @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > >    	unsigned int cnt;
> > > >    	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
> > > >    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > +	unsigned long flags;
> > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > >    	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
> > > >    		for (io = 0; io < NR_PAGE_TYPE; io++) {
> > > >    			cnt = io_lat->bio_cnt[idx][io];
> > > > @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > >    			io_lat->bio_cnt[idx][io] = 0;
> > > >    		}
> > > >    	}
> > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > >    	trace_f2fs_iostat_latency(sbi, iostat_lat);
> > > >    }
> > > > @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > >    {
> > > >    	unsigned long long iostat_diff[NR_IO_TYPE];
> > > >    	int i;
> > > > +	unsigned long flags;
> > > >    	if (time_is_after_jiffies(sbi->iostat_next_period))
> > > >    		return;
> > > >    	/* Need double check under the lock */
> > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > >    	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> > > > -		spin_unlock_bh(&sbi->iostat_lock);
> > > > +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > >    		return;
> > > >    	}
> > > >    	sbi->iostat_next_period = jiffies +
> > > > @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > >    				sbi->prev_rw_iostat[i];
> > > >    		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
> > > >    	}
> > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > >    	trace_f2fs_iostat(sbi, iostat_diff);
> > > > @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> > > >    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > >    	int i;
> > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > +	spin_lock_irq(&sbi->iostat_lock);
> > > >    	for (i = 0; i < NR_IO_TYPE; i++) {
> > > >    		sbi->rw_iostat[i] = 0;
> > > >    		sbi->prev_rw_iostat[i] = 0;
> > > >    	}
> > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > +	spin_unlock_irq(&sbi->iostat_lock);
> > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_lock_irq(&sbi->iostat_lat_lock);
> > > >    	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_unlock_irq(&sbi->iostat_lat_lock);
> > > >    }
> > > >    void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > >    			enum iostat_type type, unsigned long long io_bytes)
> > > >    {
> > > > +	unsigned long flags;
> > > > +
> > > >    	if (!sbi->iostat_enable)
> > > >    		return;
> > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > >    	sbi->rw_iostat[type] += io_bytes;
> > > >    	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> > > > @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > >    	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
> > > >    		sbi->rw_iostat[APP_READ_IO] += io_bytes;
> > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > >    	f2fs_record_iostat(sbi);
> > > >    }
> > > > @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > >    	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
> > > >    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > >    	int idx;
> > > > +	unsigned long flags;
> > > >    	if (!sbi->iostat_enable)
> > > >    		return;
> > > > @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > >    			idx = WRITE_ASYNC_IO;
> > > >    	}
> > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > >    	io_lat->sum_lat[idx][iotype] += ts_diff;
> > > >    	io_lat->bio_cnt[idx][iotype]++;
> > > >    	if (ts_diff > io_lat->peak_lat[idx][iotype])
> > > >    		io_lat->peak_lat[idx][iotype] = ts_diff;
> > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > >    }
> > > >    void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
> > > 
> > > 
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-16 16:22         ` Jaegeuk Kim
  0 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2022-06-16 16:22 UTC (permalink / raw)
  To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 06/16, Chao Yu wrote:
> On 2022/6/16 0:54, Jaegeuk Kim wrote:
> > On 06/15, Chao Yu wrote:
> > > On 2022/6/11 2:32, Daeho Jeong wrote:
> > > > From: Daeho Jeong <daehojeong@google.com>
> > > > 
> > > > Made iostat related locks safe to be called from irq context again.
> > > > 
> > > 
> > > Will be better to add a 'Fixes' line?
> > 
> > Added some tags. Thanks,
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4
> 
> It looks there are several patches not in mailing list?
> 

Which one doe you mean?

> Thanks,
> 
> > 
> > 
> > > 
> > > Thanks,
> > > 
> > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > ---
> > > >    fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
> > > >    1 file changed, 18 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> > > > index be599f31d3c4..d84c5f6cc09d 100644
> > > > --- a/fs/f2fs/iostat.c
> > > > +++ b/fs/f2fs/iostat.c
> > > > @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > >    	unsigned int cnt;
> > > >    	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
> > > >    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > +	unsigned long flags;
> > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > >    	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
> > > >    		for (io = 0; io < NR_PAGE_TYPE; io++) {
> > > >    			cnt = io_lat->bio_cnt[idx][io];
> > > > @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > >    			io_lat->bio_cnt[idx][io] = 0;
> > > >    		}
> > > >    	}
> > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > >    	trace_f2fs_iostat_latency(sbi, iostat_lat);
> > > >    }
> > > > @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > >    {
> > > >    	unsigned long long iostat_diff[NR_IO_TYPE];
> > > >    	int i;
> > > > +	unsigned long flags;
> > > >    	if (time_is_after_jiffies(sbi->iostat_next_period))
> > > >    		return;
> > > >    	/* Need double check under the lock */
> > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > >    	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> > > > -		spin_unlock_bh(&sbi->iostat_lock);
> > > > +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > >    		return;
> > > >    	}
> > > >    	sbi->iostat_next_period = jiffies +
> > > > @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > >    				sbi->prev_rw_iostat[i];
> > > >    		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
> > > >    	}
> > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > >    	trace_f2fs_iostat(sbi, iostat_diff);
> > > > @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> > > >    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > >    	int i;
> > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > +	spin_lock_irq(&sbi->iostat_lock);
> > > >    	for (i = 0; i < NR_IO_TYPE; i++) {
> > > >    		sbi->rw_iostat[i] = 0;
> > > >    		sbi->prev_rw_iostat[i] = 0;
> > > >    	}
> > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > +	spin_unlock_irq(&sbi->iostat_lock);
> > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_lock_irq(&sbi->iostat_lat_lock);
> > > >    	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_unlock_irq(&sbi->iostat_lat_lock);
> > > >    }
> > > >    void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > >    			enum iostat_type type, unsigned long long io_bytes)
> > > >    {
> > > > +	unsigned long flags;
> > > > +
> > > >    	if (!sbi->iostat_enable)
> > > >    		return;
> > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > >    	sbi->rw_iostat[type] += io_bytes;
> > > >    	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> > > > @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > >    	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
> > > >    		sbi->rw_iostat[APP_READ_IO] += io_bytes;
> > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > >    	f2fs_record_iostat(sbi);
> > > >    }
> > > > @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > >    	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
> > > >    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > >    	int idx;
> > > > +	unsigned long flags;
> > > >    	if (!sbi->iostat_enable)
> > > >    		return;
> > > > @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > >    			idx = WRITE_ASYNC_IO;
> > > >    	}
> > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > >    	io_lat->sum_lat[idx][iotype] += ts_diff;
> > > >    	io_lat->bio_cnt[idx][iotype]++;
> > > >    	if (ts_diff > io_lat->peak_lat[idx][iotype])
> > > >    		io_lat->peak_lat[idx][iotype] = ts_diff;
> > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > >    }
> > > >    void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
> > > 
> > > 
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-16 16:22         ` Jaegeuk Kim
@ 2022-06-17  1:58           ` Chao Yu
  -1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-17  1:58 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On 2022/6/17 0:22, Jaegeuk Kim wrote:
> On 06/16, Chao Yu wrote:
>> On 2022/6/16 0:54, Jaegeuk Kim wrote:
>>> On 06/15, Chao Yu wrote:
>>>> On 2022/6/11 2:32, Daeho Jeong wrote:
>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>
>>>>> Made iostat related locks safe to be called from irq context again.
>>>>>
>>>>
>>>> Will be better to add a 'Fixes' line?
>>>
>>> Added some tags. Thanks,
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4
>>
>> It looks there are several patches not in mailing list?
>>
> 
> Which one doe you mean?

f2fs: do not skip updating inode when retrying to flush node page

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=27ef61f3bf3d15caa3f4ceef60163da3f143787d

f2fs: run GCs synchronously given user requests

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=503bbcc92f0baba2a59b0a6cb4f12cf5d7141978

f2fs: attach inline_data after setting compression

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=0c837862d93c8e2e0bbb6d33efa0ff10e603c0c5

And also current patch w/ Fixes line.

Thanks,

> 
>> Thanks,
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>> ---
>>>>>     fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>>>>>     1 file changed, 18 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
>>>>> index be599f31d3c4..d84c5f6cc09d 100644
>>>>> --- a/fs/f2fs/iostat.c
>>>>> +++ b/fs/f2fs/iostat.c
>>>>> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>>>     	unsigned int cnt;
>>>>>     	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>>>>>     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>>> +	unsigned long flags;
>>>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>>>     	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>>>>>     		for (io = 0; io < NR_PAGE_TYPE; io++) {
>>>>>     			cnt = io_lat->bio_cnt[idx][io];
>>>>> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>>>     			io_lat->bio_cnt[idx][io] = 0;
>>>>>     		}
>>>>>     	}
>>>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>>>     	trace_f2fs_iostat_latency(sbi, iostat_lat);
>>>>>     }
>>>>> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>>>     {
>>>>>     	unsigned long long iostat_diff[NR_IO_TYPE];
>>>>>     	int i;
>>>>> +	unsigned long flags;
>>>>>     	if (time_is_after_jiffies(sbi->iostat_next_period))
>>>>>     		return;
>>>>>     	/* Need double check under the lock */
>>>>> -	spin_lock_bh(&sbi->iostat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>>>     	if (time_is_after_jiffies(sbi->iostat_next_period)) {
>>>>> -		spin_unlock_bh(&sbi->iostat_lock);
>>>>> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>>>     		return;
>>>>>     	}
>>>>>     	sbi->iostat_next_period = jiffies +
>>>>> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>>>     				sbi->prev_rw_iostat[i];
>>>>>     		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>>>>>     	}
>>>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>>>     	trace_f2fs_iostat(sbi, iostat_diff);
>>>>> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>>>>>     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>>>     	int i;
>>>>> -	spin_lock_bh(&sbi->iostat_lock);
>>>>> +	spin_lock_irq(&sbi->iostat_lock);
>>>>>     	for (i = 0; i < NR_IO_TYPE; i++) {
>>>>>     		sbi->rw_iostat[i] = 0;
>>>>>     		sbi->prev_rw_iostat[i] = 0;
>>>>>     	}
>>>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>>>> +	spin_unlock_irq(&sbi->iostat_lock);
>>>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_lock_irq(&sbi->iostat_lat_lock);
>>>>>     	memset(io_lat, 0, sizeof(struct iostat_lat_info));
>>>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>>>>>     }
>>>>>     void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>>>     			enum iostat_type type, unsigned long long io_bytes)
>>>>>     {
>>>>> +	unsigned long flags;
>>>>> +
>>>>>     	if (!sbi->iostat_enable)
>>>>>     		return;
>>>>> -	spin_lock_bh(&sbi->iostat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>>>     	sbi->rw_iostat[type] += io_bytes;
>>>>>     	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
>>>>> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>>>     	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>>>>>     		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>>>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>>>     	f2fs_record_iostat(sbi);
>>>>>     }
>>>>> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>>>     	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>>>>>     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>>>     	int idx;
>>>>> +	unsigned long flags;
>>>>>     	if (!sbi->iostat_enable)
>>>>>     		return;
>>>>> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>>>     			idx = WRITE_ASYNC_IO;
>>>>>     	}
>>>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>>>     	io_lat->sum_lat[idx][iotype] += ts_diff;
>>>>>     	io_lat->bio_cnt[idx][iotype]++;
>>>>>     	if (ts_diff > io_lat->peak_lat[idx][iotype])
>>>>>     		io_lat->peak_lat[idx][iotype] = ts_diff;
>>>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>>>     }
>>>>>     void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-f2fs-devel mailing list
>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-17  1:58           ` Chao Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-17  1:58 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 2022/6/17 0:22, Jaegeuk Kim wrote:
> On 06/16, Chao Yu wrote:
>> On 2022/6/16 0:54, Jaegeuk Kim wrote:
>>> On 06/15, Chao Yu wrote:
>>>> On 2022/6/11 2:32, Daeho Jeong wrote:
>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>
>>>>> Made iostat related locks safe to be called from irq context again.
>>>>>
>>>>
>>>> Will be better to add a 'Fixes' line?
>>>
>>> Added some tags. Thanks,
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4
>>
>> It looks there are several patches not in mailing list?
>>
> 
> Which one doe you mean?

f2fs: do not skip updating inode when retrying to flush node page

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=27ef61f3bf3d15caa3f4ceef60163da3f143787d

f2fs: run GCs synchronously given user requests

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=503bbcc92f0baba2a59b0a6cb4f12cf5d7141978

f2fs: attach inline_data after setting compression

https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=0c837862d93c8e2e0bbb6d33efa0ff10e603c0c5

And also current patch w/ Fixes line.

Thanks,

> 
>> Thanks,
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>> ---
>>>>>     fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>>>>>     1 file changed, 18 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
>>>>> index be599f31d3c4..d84c5f6cc09d 100644
>>>>> --- a/fs/f2fs/iostat.c
>>>>> +++ b/fs/f2fs/iostat.c
>>>>> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>>>     	unsigned int cnt;
>>>>>     	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>>>>>     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>>> +	unsigned long flags;
>>>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>>>     	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>>>>>     		for (io = 0; io < NR_PAGE_TYPE; io++) {
>>>>>     			cnt = io_lat->bio_cnt[idx][io];
>>>>> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>>>     			io_lat->bio_cnt[idx][io] = 0;
>>>>>     		}
>>>>>     	}
>>>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>>>     	trace_f2fs_iostat_latency(sbi, iostat_lat);
>>>>>     }
>>>>> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>>>     {
>>>>>     	unsigned long long iostat_diff[NR_IO_TYPE];
>>>>>     	int i;
>>>>> +	unsigned long flags;
>>>>>     	if (time_is_after_jiffies(sbi->iostat_next_period))
>>>>>     		return;
>>>>>     	/* Need double check under the lock */
>>>>> -	spin_lock_bh(&sbi->iostat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>>>     	if (time_is_after_jiffies(sbi->iostat_next_period)) {
>>>>> -		spin_unlock_bh(&sbi->iostat_lock);
>>>>> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>>>     		return;
>>>>>     	}
>>>>>     	sbi->iostat_next_period = jiffies +
>>>>> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>>>     				sbi->prev_rw_iostat[i];
>>>>>     		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>>>>>     	}
>>>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>>>     	trace_f2fs_iostat(sbi, iostat_diff);
>>>>> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>>>>>     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>>>     	int i;
>>>>> -	spin_lock_bh(&sbi->iostat_lock);
>>>>> +	spin_lock_irq(&sbi->iostat_lock);
>>>>>     	for (i = 0; i < NR_IO_TYPE; i++) {
>>>>>     		sbi->rw_iostat[i] = 0;
>>>>>     		sbi->prev_rw_iostat[i] = 0;
>>>>>     	}
>>>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>>>> +	spin_unlock_irq(&sbi->iostat_lock);
>>>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_lock_irq(&sbi->iostat_lat_lock);
>>>>>     	memset(io_lat, 0, sizeof(struct iostat_lat_info));
>>>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>>>>>     }
>>>>>     void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>>>     			enum iostat_type type, unsigned long long io_bytes)
>>>>>     {
>>>>> +	unsigned long flags;
>>>>> +
>>>>>     	if (!sbi->iostat_enable)
>>>>>     		return;
>>>>> -	spin_lock_bh(&sbi->iostat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>>>     	sbi->rw_iostat[type] += io_bytes;
>>>>>     	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
>>>>> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>>>     	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>>>>>     		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>>>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>>>     	f2fs_record_iostat(sbi);
>>>>>     }
>>>>> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>>>     	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>>>>>     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>>>     	int idx;
>>>>> +	unsigned long flags;
>>>>>     	if (!sbi->iostat_enable)
>>>>>     		return;
>>>>> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>>>     			idx = WRITE_ASYNC_IO;
>>>>>     	}
>>>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>>>     	io_lat->sum_lat[idx][iotype] += ts_diff;
>>>>>     	io_lat->bio_cnt[idx][iotype]++;
>>>>>     	if (ts_diff > io_lat->peak_lat[idx][iotype])
>>>>>     		io_lat->peak_lat[idx][iotype] = ts_diff;
>>>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>>>     }
>>>>>     void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-f2fs-devel mailing list
>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-17  1:58           ` Chao Yu
@ 2022-06-17 22:31             ` Jaegeuk Kim
  -1 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2022-06-17 22:31 UTC (permalink / raw)
  To: Chao Yu
  Cc: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On 06/17, Chao Yu wrote:
> On 2022/6/17 0:22, Jaegeuk Kim wrote:
> > On 06/16, Chao Yu wrote:
> > > On 2022/6/16 0:54, Jaegeuk Kim wrote:
> > > > On 06/15, Chao Yu wrote:
> > > > > On 2022/6/11 2:32, Daeho Jeong wrote:
> > > > > > From: Daeho Jeong <daehojeong@google.com>
> > > > > > 
> > > > > > Made iostat related locks safe to be called from irq context again.
> > > > > > 
> > > > > 
> > > > > Will be better to add a 'Fixes' line?
> > > > 
> > > > Added some tags. Thanks,
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4
> > > 
> > > It looks there are several patches not in mailing list?
> > > 
> > 
> > Which one doe you mean?
> 
> f2fs: do not skip updating inode when retrying to flush node page
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=27ef61f3bf3d15caa3f4ceef60163da3f143787d
> 
> f2fs: run GCs synchronously given user requests
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=503bbcc92f0baba2a59b0a6cb4f12cf5d7141978
> 
> f2fs: attach inline_data after setting compression
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=0c837862d93c8e2e0bbb6d33efa0ff10e603c0c5
> 
> And also current patch w/ Fixes line.

Ah, I found I sent them to LKML only.

> 
> Thanks,
> 
> > 
> > > Thanks,
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > > > ---
> > > > > >     fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
> > > > > >     1 file changed, 18 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> > > > > > index be599f31d3c4..d84c5f6cc09d 100644
> > > > > > --- a/fs/f2fs/iostat.c
> > > > > > +++ b/fs/f2fs/iostat.c
> > > > > > @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > > > >     	unsigned int cnt;
> > > > > >     	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
> > > > > >     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > > > +	unsigned long flags;
> > > > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > > > >     	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
> > > > > >     		for (io = 0; io < NR_PAGE_TYPE; io++) {
> > > > > >     			cnt = io_lat->bio_cnt[idx][io];
> > > > > > @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > > > >     			io_lat->bio_cnt[idx][io] = 0;
> > > > > >     		}
> > > > > >     	}
> > > > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > > > >     	trace_f2fs_iostat_latency(sbi, iostat_lat);
> > > > > >     }
> > > > > > @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > > > >     {
> > > > > >     	unsigned long long iostat_diff[NR_IO_TYPE];
> > > > > >     	int i;
> > > > > > +	unsigned long flags;
> > > > > >     	if (time_is_after_jiffies(sbi->iostat_next_period))
> > > > > >     		return;
> > > > > >     	/* Need double check under the lock */
> > > > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > > > >     	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> > > > > > -		spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > > > >     		return;
> > > > > >     	}
> > > > > >     	sbi->iostat_next_period = jiffies +
> > > > > > @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > > > >     				sbi->prev_rw_iostat[i];
> > > > > >     		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
> > > > > >     	}
> > > > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > > > >     	trace_f2fs_iostat(sbi, iostat_diff);
> > > > > > @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> > > > > >     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > > >     	int i;
> > > > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > > > +	spin_lock_irq(&sbi->iostat_lock);
> > > > > >     	for (i = 0; i < NR_IO_TYPE; i++) {
> > > > > >     		sbi->rw_iostat[i] = 0;
> > > > > >     		sbi->prev_rw_iostat[i] = 0;
> > > > > >     	}
> > > > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +	spin_unlock_irq(&sbi->iostat_lock);
> > > > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_lock_irq(&sbi->iostat_lat_lock);
> > > > > >     	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> > > > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_unlock_irq(&sbi->iostat_lat_lock);
> > > > > >     }
> > > > > >     void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > > > >     			enum iostat_type type, unsigned long long io_bytes)
> > > > > >     {
> > > > > > +	unsigned long flags;
> > > > > > +
> > > > > >     	if (!sbi->iostat_enable)
> > > > > >     		return;
> > > > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > > > >     	sbi->rw_iostat[type] += io_bytes;
> > > > > >     	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> > > > > > @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > > > >     	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
> > > > > >     		sbi->rw_iostat[APP_READ_IO] += io_bytes;
> > > > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > > > >     	f2fs_record_iostat(sbi);
> > > > > >     }
> > > > > > @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > > > >     	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
> > > > > >     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > > >     	int idx;
> > > > > > +	unsigned long flags;
> > > > > >     	if (!sbi->iostat_enable)
> > > > > >     		return;
> > > > > > @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > > > >     			idx = WRITE_ASYNC_IO;
> > > > > >     	}
> > > > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > > > >     	io_lat->sum_lat[idx][iotype] += ts_diff;
> > > > > >     	io_lat->bio_cnt[idx][iotype]++;
> > > > > >     	if (ts_diff > io_lat->peak_lat[idx][iotype])
> > > > > >     		io_lat->peak_lat[idx][iotype] = ts_diff;
> > > > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > > > >     }
> > > > > >     void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
> > > > > 
> > > > > 
> > > > > _______________________________________________
> > > > > Linux-f2fs-devel mailing list
> > > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-17 22:31             ` Jaegeuk Kim
  0 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2022-06-17 22:31 UTC (permalink / raw)
  To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 06/17, Chao Yu wrote:
> On 2022/6/17 0:22, Jaegeuk Kim wrote:
> > On 06/16, Chao Yu wrote:
> > > On 2022/6/16 0:54, Jaegeuk Kim wrote:
> > > > On 06/15, Chao Yu wrote:
> > > > > On 2022/6/11 2:32, Daeho Jeong wrote:
> > > > > > From: Daeho Jeong <daehojeong@google.com>
> > > > > > 
> > > > > > Made iostat related locks safe to be called from irq context again.
> > > > > > 
> > > > > 
> > > > > Will be better to add a 'Fixes' line?
> > > > 
> > > > Added some tags. Thanks,
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4
> > > 
> > > It looks there are several patches not in mailing list?
> > > 
> > 
> > Which one doe you mean?
> 
> f2fs: do not skip updating inode when retrying to flush node page
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=27ef61f3bf3d15caa3f4ceef60163da3f143787d
> 
> f2fs: run GCs synchronously given user requests
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=503bbcc92f0baba2a59b0a6cb4f12cf5d7141978
> 
> f2fs: attach inline_data after setting compression
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=0c837862d93c8e2e0bbb6d33efa0ff10e603c0c5
> 
> And also current patch w/ Fixes line.

Ah, I found I sent them to LKML only.

> 
> Thanks,
> 
> > 
> > > Thanks,
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > > > ---
> > > > > >     fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
> > > > > >     1 file changed, 18 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> > > > > > index be599f31d3c4..d84c5f6cc09d 100644
> > > > > > --- a/fs/f2fs/iostat.c
> > > > > > +++ b/fs/f2fs/iostat.c
> > > > > > @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > > > >     	unsigned int cnt;
> > > > > >     	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
> > > > > >     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > > > +	unsigned long flags;
> > > > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > > > >     	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
> > > > > >     		for (io = 0; io < NR_PAGE_TYPE; io++) {
> > > > > >     			cnt = io_lat->bio_cnt[idx][io];
> > > > > > @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
> > > > > >     			io_lat->bio_cnt[idx][io] = 0;
> > > > > >     		}
> > > > > >     	}
> > > > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > > > >     	trace_f2fs_iostat_latency(sbi, iostat_lat);
> > > > > >     }
> > > > > > @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > > > >     {
> > > > > >     	unsigned long long iostat_diff[NR_IO_TYPE];
> > > > > >     	int i;
> > > > > > +	unsigned long flags;
> > > > > >     	if (time_is_after_jiffies(sbi->iostat_next_period))
> > > > > >     		return;
> > > > > >     	/* Need double check under the lock */
> > > > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > > > >     	if (time_is_after_jiffies(sbi->iostat_next_period)) {
> > > > > > -		spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > > > >     		return;
> > > > > >     	}
> > > > > >     	sbi->iostat_next_period = jiffies +
> > > > > > @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
> > > > > >     				sbi->prev_rw_iostat[i];
> > > > > >     		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
> > > > > >     	}
> > > > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > > > >     	trace_f2fs_iostat(sbi, iostat_diff);
> > > > > > @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> > > > > >     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > > >     	int i;
> > > > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > > > +	spin_lock_irq(&sbi->iostat_lock);
> > > > > >     	for (i = 0; i < NR_IO_TYPE; i++) {
> > > > > >     		sbi->rw_iostat[i] = 0;
> > > > > >     		sbi->prev_rw_iostat[i] = 0;
> > > > > >     	}
> > > > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +	spin_unlock_irq(&sbi->iostat_lock);
> > > > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_lock_irq(&sbi->iostat_lat_lock);
> > > > > >     	memset(io_lat, 0, sizeof(struct iostat_lat_info));
> > > > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_unlock_irq(&sbi->iostat_lat_lock);
> > > > > >     }
> > > > > >     void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > > > >     			enum iostat_type type, unsigned long long io_bytes)
> > > > > >     {
> > > > > > +	unsigned long flags;
> > > > > > +
> > > > > >     	if (!sbi->iostat_enable)
> > > > > >     		return;
> > > > > > -	spin_lock_bh(&sbi->iostat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lock, flags);
> > > > > >     	sbi->rw_iostat[type] += io_bytes;
> > > > > >     	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
> > > > > > @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
> > > > > >     	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
> > > > > >     		sbi->rw_iostat[APP_READ_IO] += io_bytes;
> > > > > > -	spin_unlock_bh(&sbi->iostat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
> > > > > >     	f2fs_record_iostat(sbi);
> > > > > >     }
> > > > > > @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > > > >     	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
> > > > > >     	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
> > > > > >     	int idx;
> > > > > > +	unsigned long flags;
> > > > > >     	if (!sbi->iostat_enable)
> > > > > >     		return;
> > > > > > @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
> > > > > >     			idx = WRITE_ASYNC_IO;
> > > > > >     	}
> > > > > > -	spin_lock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
> > > > > >     	io_lat->sum_lat[idx][iotype] += ts_diff;
> > > > > >     	io_lat->bio_cnt[idx][iotype]++;
> > > > > >     	if (ts_diff > io_lat->peak_lat[idx][iotype])
> > > > > >     		io_lat->peak_lat[idx][iotype] = ts_diff;
> > > > > > -	spin_unlock_bh(&sbi->iostat_lat_lock);
> > > > > > +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
> > > > > >     }
> > > > > >     void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
> > > > > 
> > > > > 
> > > > > _______________________________________________
> > > > > Linux-f2fs-devel mailing list
> > > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
  2022-06-15 16:54     ` Jaegeuk Kim
@ 2022-06-19  0:26       ` Chao Yu
  -1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-19  0:26 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

On 2022/6/16 0:54, Jaegeuk Kim wrote:
> On 06/15, Chao Yu wrote:
>> On 2022/6/11 2:32, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> Made iostat related locks safe to be called from irq context again.
>>>
>>
>> Will be better to add a 'Fixes' line?
> 
> Added some tags. Thanks,
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

> 
> 
>>
>> Thanks,
>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>>    fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>>>    1 file changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
>>> index be599f31d3c4..d84c5f6cc09d 100644
>>> --- a/fs/f2fs/iostat.c
>>> +++ b/fs/f2fs/iostat.c
>>> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    	unsigned int cnt;
>>>    	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>> +	unsigned long flags;
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>>>    		for (io = 0; io < NR_PAGE_TYPE; io++) {
>>>    			cnt = io_lat->bio_cnt[idx][io];
>>> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    			io_lat->bio_cnt[idx][io] = 0;
>>>    		}
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    	trace_f2fs_iostat_latency(sbi, iostat_lat);
>>>    }
>>> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    {
>>>    	unsigned long long iostat_diff[NR_IO_TYPE];
>>>    	int i;
>>> +	unsigned long flags;
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period))
>>>    		return;
>>>    	/* Need double check under the lock */
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period)) {
>>> -		spin_unlock_bh(&sbi->iostat_lock);
>>> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    		return;
>>>    	}
>>>    	sbi->iostat_next_period = jiffies +
>>> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    				sbi->prev_rw_iostat[i];
>>>    		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	trace_f2fs_iostat(sbi, iostat_diff);
>>> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int i;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lock);
>>>    	for (i = 0; i < NR_IO_TYPE; i++) {
>>>    		sbi->rw_iostat[i] = 0;
>>>    		sbi->prev_rw_iostat[i] = 0;
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lock);
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lat_lock);
>>>    	memset(io_lat, 0, sizeof(struct iostat_lat_info));
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>>>    }
>>>    void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    			enum iostat_type type, unsigned long long io_bytes)
>>>    {
>>> +	unsigned long flags;
>>> +
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	sbi->rw_iostat[type] += io_bytes;
>>>    	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
>>> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>>>    		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	f2fs_record_iostat(sbi);
>>>    }
>>> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int idx;
>>> +	unsigned long flags;
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    			idx = WRITE_ASYNC_IO;
>>>    	}
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	io_lat->sum_lat[idx][iotype] += ts_diff;
>>>    	io_lat->bio_cnt[idx][iotype]++;
>>>    	if (ts_diff > io_lat->peak_lat[idx][iotype])
>>>    		io_lat->peak_lat[idx][iotype] = ts_diff;
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    }
>>>    void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix iostat related lock protection
@ 2022-06-19  0:26       ` Chao Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2022-06-19  0:26 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel

On 2022/6/16 0:54, Jaegeuk Kim wrote:
> On 06/15, Chao Yu wrote:
>> On 2022/6/11 2:32, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> Made iostat related locks safe to be called from irq context again.
>>>
>>
>> Will be better to add a 'Fixes' line?
> 
> Added some tags. Thanks,
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=f8ed39ad779fbc5d37d08e83643384fc06e4bae4

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

> 
> 
>>
>> Thanks,
>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>>    fs/f2fs/iostat.c | 31 ++++++++++++++++++-------------
>>>    1 file changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
>>> index be599f31d3c4..d84c5f6cc09d 100644
>>> --- a/fs/f2fs/iostat.c
>>> +++ b/fs/f2fs/iostat.c
>>> @@ -91,8 +91,9 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    	unsigned int cnt;
>>>    	struct f2fs_iostat_latency iostat_lat[MAX_IO_TYPE][NR_PAGE_TYPE];
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>> +	unsigned long flags;
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	for (idx = 0; idx < MAX_IO_TYPE; idx++) {
>>>    		for (io = 0; io < NR_PAGE_TYPE; io++) {
>>>    			cnt = io_lat->bio_cnt[idx][io];
>>> @@ -106,7 +107,7 @@ static inline void __record_iostat_latency(struct f2fs_sb_info *sbi)
>>>    			io_lat->bio_cnt[idx][io] = 0;
>>>    		}
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    	trace_f2fs_iostat_latency(sbi, iostat_lat);
>>>    }
>>> @@ -115,14 +116,15 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    {
>>>    	unsigned long long iostat_diff[NR_IO_TYPE];
>>>    	int i;
>>> +	unsigned long flags;
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period))
>>>    		return;
>>>    	/* Need double check under the lock */
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	if (time_is_after_jiffies(sbi->iostat_next_period)) {
>>> -		spin_unlock_bh(&sbi->iostat_lock);
>>> +		spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    		return;
>>>    	}
>>>    	sbi->iostat_next_period = jiffies +
>>> @@ -133,7 +135,7 @@ static inline void f2fs_record_iostat(struct f2fs_sb_info *sbi)
>>>    				sbi->prev_rw_iostat[i];
>>>    		sbi->prev_rw_iostat[i] = sbi->rw_iostat[i];
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	trace_f2fs_iostat(sbi, iostat_diff);
>>> @@ -145,25 +147,27 @@ void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int i;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lock);
>>>    	for (i = 0; i < NR_IO_TYPE; i++) {
>>>    		sbi->rw_iostat[i] = 0;
>>>    		sbi->prev_rw_iostat[i] = 0;
>>>    	}
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lock);
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irq(&sbi->iostat_lat_lock);
>>>    	memset(io_lat, 0, sizeof(struct iostat_lat_info));
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irq(&sbi->iostat_lat_lock);
>>>    }
>>>    void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    			enum iostat_type type, unsigned long long io_bytes)
>>>    {
>>> +	unsigned long flags;
>>> +
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> -	spin_lock_bh(&sbi->iostat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lock, flags);
>>>    	sbi->rw_iostat[type] += io_bytes;
>>>    	if (type == APP_BUFFERED_IO || type == APP_DIRECT_IO)
>>> @@ -172,7 +176,7 @@ void f2fs_update_iostat(struct f2fs_sb_info *sbi,
>>>    	if (type == APP_BUFFERED_READ_IO || type == APP_DIRECT_READ_IO)
>>>    		sbi->rw_iostat[APP_READ_IO] += io_bytes;
>>> -	spin_unlock_bh(&sbi->iostat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lock, flags);
>>>    	f2fs_record_iostat(sbi);
>>>    }
>>> @@ -185,6 +189,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    	struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>>>    	struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>>>    	int idx;
>>> +	unsigned long flags;
>>>    	if (!sbi->iostat_enable)
>>>    		return;
>>> @@ -202,12 +207,12 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>>>    			idx = WRITE_ASYNC_IO;
>>>    	}
>>> -	spin_lock_bh(&sbi->iostat_lat_lock);
>>> +	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>>>    	io_lat->sum_lat[idx][iotype] += ts_diff;
>>>    	io_lat->bio_cnt[idx][iotype]++;
>>>    	if (ts_diff > io_lat->peak_lat[idx][iotype])
>>>    		io_lat->peak_lat[idx][iotype] = ts_diff;
>>> -	spin_unlock_bh(&sbi->iostat_lat_lock);
>>> +	spin_unlock_irqrestore(&sbi->iostat_lat_lock, flags);
>>>    }
>>>    void iostat_update_and_unbind_ctx(struct bio *bio, int rw)
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2022-06-19  0:26 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-10 18:32 [PATCH] f2fs: fix iostat related lock protection Daeho Jeong
2022-06-10 18:32 ` [f2fs-dev] " Daeho Jeong
2022-06-15  7:54 ` Chao Yu
2022-06-15  7:54   ` Chao Yu
2022-06-15 16:54   ` Jaegeuk Kim
2022-06-15 16:54     ` Jaegeuk Kim
2022-06-16  2:11     ` Chao Yu
2022-06-16  2:11       ` Chao Yu
2022-06-16 16:22       ` Jaegeuk Kim
2022-06-16 16:22         ` Jaegeuk Kim
2022-06-17  1:58         ` Chao Yu
2022-06-17  1:58           ` Chao Yu
2022-06-17 22:31           ` Jaegeuk Kim
2022-06-17 22:31             ` Jaegeuk Kim
2022-06-19  0:26     ` Chao Yu
2022-06-19  0:26       ` Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.