[PATCH v3] exfat: check disk status during buffer write

[PATCH v3] exfat: check disk status during buffer write

[Dongliang Cui]

We found that when writing a large file through buffer write, if the
disk is inaccessible, exFAT does not return an error normally, which
leads to the writing process not stopping properly.

To easily reproduce this issue, you can follow the steps below:

1. format a device to exFAT and then mount (with a full disk erase)
2. dd if=/dev/zero of=/exfat_mount/test.img bs=1M count=8192
3. eject the device

You may find that the dd process does not stop immediately and may
continue for a long time.

The root cause of this issue is that during buffer write process,
exFAT does not need to access the disk to look up directory entries
or the FAT table (whereas FAT would do) every time data is written.
Instead, exFAT simply marks the buffer as dirty and returns,
delegating the writeback operation to the writeback process.

If the disk cannot be accessed at this time, the error will only be
returned to the writeback process, and the original process will not
receive the error, so it cannot be returned to the user side.

When the disk cannot be accessed normally, an error should be returned
to stop the writing process.

Signed-off-by: Dongliang Cui <dongliang.cui@unisoc.com>
Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
Changes in v3:
 - Implement .shutdown to monitor disk status.
---
 fs/exfat/exfat_fs.h | 10 ++++++++++
 fs/exfat/inode.c    |  3 +++
 fs/exfat/super.c    | 11 +++++++++++
 3 files changed, 24 insertions(+)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index ecc5db952deb..c6cf36070aa3 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -148,6 +148,9 @@ enum {
 #define DIR_CACHE_SIZE		\
 	(DIV_ROUND_UP(EXFAT_DEN_TO_B(ES_MAX_ENTRY_NUM), SECTOR_SIZE) + 1)
 
+/* Superblock flags */
+#define EXFAT_FLAGS_SHUTDOWN	1
+
 struct exfat_dentry_namebuf {
 	char *lfn;
 	int lfnbuf_len; /* usually MAX_UNINAME_BUF_SIZE */
@@ -267,6 +270,8 @@ struct exfat_sb_info {
 	unsigned int clu_srch_ptr; /* cluster search pointer */
 	unsigned int used_clusters; /* number of used clusters */
 
+	unsigned long s_exfat_flags; /* Exfat superblock flags */
+
 	struct mutex s_lock; /* superblock lock */
 	struct mutex bitmap_lock; /* bitmap lock */
 	struct exfat_mount_options options;
@@ -338,6 +343,11 @@ static inline struct exfat_inode_info *EXFAT_I(struct inode *inode)
 	return container_of(inode, struct exfat_inode_info, vfs_inode);
 }
 
+static inline int exfat_forced_shutdown(struct super_block *sb)
+{
+	return test_bit(EXFAT_FLAGS_SHUTDOWN, &EXFAT_SB(sb)->s_exfat_flags);
+}
+
 /*
  * If ->i_mode can't hold 0222 (i.e. ATTR_RO), we use ->i_attrs to
  * save ATTR_RO instead of ->i_mode.
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index dd894e558c91..b1b814183494 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -452,6 +452,9 @@ static int exfat_write_begin(struct file *file, struct address_space *mapping,
 {
 	int ret;
 
+	if (unlikely(exfat_forced_shutdown(mapping->host->i_sb)))
+		return -EIO;
+
 	*pagep = NULL;
 	ret = block_write_begin(mapping, pos, len, pagep, exfat_get_block);
 
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 323ecebe6f0e..9d7d9c4ba55a 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -167,6 +167,16 @@ static int exfat_show_options(struct seq_file *m, struct dentry *root)
 	return 0;
 }
 
+static void exfat_shutdown(struct super_block *sb)
+{
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+
+	if (exfat_forced_shutdown(sb))
+		return;
+
+	set_bit(EXFAT_FLAGS_SHUTDOWN, &sbi->s_exfat_flags);
+}
+
 static struct inode *exfat_alloc_inode(struct super_block *sb)
 {
 	struct exfat_inode_info *ei;
@@ -193,6 +203,7 @@ static const struct super_operations exfat_sops = {
 	.sync_fs	= exfat_sync_fs,
 	.statfs		= exfat_statfs,
 	.show_options	= exfat_show_options,
+	.shutdown	= exfat_shutdown,
 };
 
 enum {
-- 
2.25.1

Re: [PATCH v3] exfat: check disk status during buffer write

[崔东亮 (Dongliang Cui)]

Loop more

-----邮件原件-----
发件人: 崔东亮 (Dongliang Cui) <Dongliang.Cui@unisoc.com> 
发送时间: 2024年7月31日 10:27
收件人: linkinjeon@kernel.org; sj1557.seo@samsung.com; hch@infradead.org; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
抄送: niuzhiguo84@gmail.com; 王皓 (Hao_hao Wang) <Hao_hao.Wang@unisoc.com>; 王科 (Ke Wang) <Ke.Wang@unisoc.com>; 崔东亮 (Dongliang Cui) <Dongliang.Cui@unisoc.com>; 牛志国 (Zhiguo Niu) <Zhiguo.Niu@unisoc.com>
主题: [PATCH v3] exfat: check disk status during buffer write

We found that when writing a large file through buffer write, if the disk is inaccessible, exFAT does not return an error normally, which leads to the writing process not stopping properly.

To easily reproduce this issue, you can follow the steps below:

1. format a device to exFAT and then mount (with a full disk erase) 2. dd if=/dev/zero of=/exfat_mount/test.img bs=1M count=8192 3. eject the device

You may find that the dd process does not stop immediately and may continue for a long time.

The root cause of this issue is that during buffer write process, exFAT does not need to access the disk to look up directory entries or the FAT table (whereas FAT would do) every time data is written.
Instead, exFAT simply marks the buffer as dirty and returns, delegating the writeback operation to the writeback process.

If the disk cannot be accessed at this time, the error will only be returned to the writeback process, and the original process will not receive the error, so it cannot be returned to the user side.

When the disk cannot be accessed normally, an error should be returned to stop the writing process.

Signed-off-by: Dongliang Cui <dongliang.cui@unisoc.com>
Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
Changes in v3:
 - Implement .shutdown to monitor disk status.
---
 fs/exfat/exfat_fs.h | 10 ++++++++++
 fs/exfat/inode.c    |  3 +++
 fs/exfat/super.c    | 11 +++++++++++
 3 files changed, 24 insertions(+)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index ecc5db952deb..c6cf36070aa3 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -148,6 +148,9 @@ enum {
 #define DIR_CACHE_SIZE		\
 	(DIV_ROUND_UP(EXFAT_DEN_TO_B(ES_MAX_ENTRY_NUM), SECTOR_SIZE) + 1)
 
+/* Superblock flags */
+#define EXFAT_FLAGS_SHUTDOWN	1
+
 struct exfat_dentry_namebuf {
 	char *lfn;
 	int lfnbuf_len; /* usually MAX_UNINAME_BUF_SIZE */ @@ -267,6 +270,8 @@ struct exfat_sb_info {
 	unsigned int clu_srch_ptr; /* cluster search pointer */
 	unsigned int used_clusters; /* number of used clusters */
 
+	unsigned long s_exfat_flags; /* Exfat superblock flags */
+
 	struct mutex s_lock; /* superblock lock */
 	struct mutex bitmap_lock; /* bitmap lock */
 	struct exfat_mount_options options;
@@ -338,6 +343,11 @@ static inline struct exfat_inode_info *EXFAT_I(struct inode *inode)
 	return container_of(inode, struct exfat_inode_info, vfs_inode);  }
 
+static inline int exfat_forced_shutdown(struct super_block *sb) {
+	return test_bit(EXFAT_FLAGS_SHUTDOWN, &EXFAT_SB(sb)->s_exfat_flags); }
+
 /*
  * If ->i_mode can't hold 0222 (i.e. ATTR_RO), we use ->i_attrs to
  * save ATTR_RO instead of ->i_mode.
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index dd894e558c91..b1b814183494 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -452,6 +452,9 @@ static int exfat_write_begin(struct file *file, struct address_space *mapping,  {
 	int ret;
 
+	if (unlikely(exfat_forced_shutdown(mapping->host->i_sb)))
+		return -EIO;
+
 	*pagep = NULL;
 	ret = block_write_begin(mapping, pos, len, pagep, exfat_get_block);
 
diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 323ecebe6f0e..9d7d9c4ba55a 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -167,6 +167,16 @@ static int exfat_show_options(struct seq_file *m, struct dentry *root)
 	return 0;
 }
 
+static void exfat_shutdown(struct super_block *sb) {
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+
+	if (exfat_forced_shutdown(sb))
+		return;
+
+	set_bit(EXFAT_FLAGS_SHUTDOWN, &sbi->s_exfat_flags); }
+
 static struct inode *exfat_alloc_inode(struct super_block *sb)  {
 	struct exfat_inode_info *ei;
@@ -193,6 +203,7 @@ static const struct super_operations exfat_sops = {
 	.sync_fs	= exfat_sync_fs,
 	.statfs		= exfat_statfs,
 	.show_options	= exfat_show_options,
+	.shutdown	= exfat_shutdown,
 };
 
 enum {
--
2.25.1

Re: [PATCH v3] exfat: check disk status during buffer write

[Namjae Jeon]

2024년 7월 31일 (수) 오전 11:29, Dongliang Cui <dongliang.cui@unisoc.com>님이 작성:
>
> We found that when writing a large file through buffer write, if the
> disk is inaccessible, exFAT does not return an error normally, which
> leads to the writing process not stopping properly.
>
> To easily reproduce this issue, you can follow the steps below:
>
> 1. format a device to exFAT and then mount (with a full disk erase)
> 2. dd if=/dev/zero of=/exfat_mount/test.img bs=1M count=8192
> 3. eject the device
>
> You may find that the dd process does not stop immediately and may
> continue for a long time.
>
> The root cause of this issue is that during buffer write process,
> exFAT does not need to access the disk to look up directory entries
> or the FAT table (whereas FAT would do) every time data is written.
> Instead, exFAT simply marks the buffer as dirty and returns,
> delegating the writeback operation to the writeback process.
>
> If the disk cannot be accessed at this time, the error will only be
> returned to the writeback process, and the original process will not
> receive the error, so it cannot be returned to the user side.
>
> When the disk cannot be accessed normally, an error should be returned
> to stop the writing process.
>
> Signed-off-by: Dongliang Cui <dongliang.cui@unisoc.com>
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> ---
> Changes in v3:
>  - Implement .shutdown to monitor disk status.
> ---
>  fs/exfat/exfat_fs.h | 10 ++++++++++
>  fs/exfat/inode.c    |  3 +++
>  fs/exfat/super.c    | 11 +++++++++++
>  3 files changed, 24 insertions(+)
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index ecc5db952deb..c6cf36070aa3 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -148,6 +148,9 @@ enum {
>  #define DIR_CACHE_SIZE         \
>         (DIV_ROUND_UP(EXFAT_DEN_TO_B(ES_MAX_ENTRY_NUM), SECTOR_SIZE) + 1)
>
> +/* Superblock flags */
> +#define EXFAT_FLAGS_SHUTDOWN   1
> +
>  struct exfat_dentry_namebuf {
>         char *lfn;
>         int lfnbuf_len; /* usually MAX_UNINAME_BUF_SIZE */
> @@ -267,6 +270,8 @@ struct exfat_sb_info {
>         unsigned int clu_srch_ptr; /* cluster search pointer */
>         unsigned int used_clusters; /* number of used clusters */
>
> +       unsigned long s_exfat_flags; /* Exfat superblock flags */
> +
>         struct mutex s_lock; /* superblock lock */
>         struct mutex bitmap_lock; /* bitmap lock */
>         struct exfat_mount_options options;
> @@ -338,6 +343,11 @@ static inline struct exfat_inode_info *EXFAT_I(struct inode *inode)
>         return container_of(inode, struct exfat_inode_info, vfs_inode);
>  }
>
> +static inline int exfat_forced_shutdown(struct super_block *sb)
> +{
> +       return test_bit(EXFAT_FLAGS_SHUTDOWN, &EXFAT_SB(sb)->s_exfat_flags);
> +}
> +
>  /*
>   * If ->i_mode can't hold 0222 (i.e. ATTR_RO), we use ->i_attrs to
>   * save ATTR_RO instead of ->i_mode.
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
> index dd894e558c91..b1b814183494 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -452,6 +452,9 @@ static int exfat_write_begin(struct file *file, struct address_space *mapping,
>  {
>         int ret;
>
> +       if (unlikely(exfat_forced_shutdown(mapping->host->i_sb)))
> +               return -EIO;
We need to add this to other write operations(exfat_create, unlink,
mkdir, rmdir, rename, setattr, writepages)
as well as exfat_write_begin().
Thanks.

Re: [PATCH v3] exfat: check disk status during buffer write

[Christoph Hellwig]

Besides the additional checks for the shutdown flag already mentioned
the subject is now incorrect I think, it should talk about implementing
shutdown handling.

In case you haven't done so yet, please also see if exfat now passes
the various testcases in xfstests that exercise the shutdown path.

Otherwise this looks reasonable to me, thanks for the work!

Re: [PATCH v3] exfat: check disk status during buffer write

[崔东亮 (Dongliang Cui)]

Besides the additional checks for the shutdown flag already mentioned the subject is now incorrect I think, it should talk about implementing shutdown handling.

In case you haven't done so yet, please also see if exfat now passes the various testcases in xfstests that exercise the shutdown path.

Otherwise this looks reasonable to me, thanks for the work!

Hi Christoph,

Thank you for your suggestion. I think the current patch is primarily aimed at addressing the issue of hotplug and ensuring writers are notified when a device has been ejected.

Previously, exfat didn't have a shutdown process inherently, and hotplug didn't pose any significant issues, except for the one we're discussing in this email.

Therefore, regarding what specific actions should be taken during shutdown, I would appreciate your input or any suggestions from Sungjong and Namjae. 

Additionally, can the shutdown handling be supplemented with another patch if there is indeed a need to implement some exfat shutdown processes?

HI Sungjong and Namjae.

Based on the above, what do you think, or do you have any suggestions?

Re: [PATCH v3] exfat: check disk status during buffer write

[Namjae Jeon]

2024년 8월 1일 (목) 오후 2:30, 崔东亮 (Dongliang Cui) <Dongliang.Cui@unisoc.com>님이 작성:
>
> Besides the additional checks for the shutdown flag already mentioned the subject is now incorrect I think, it should talk about implementing shutdown handling.
>
> In case you haven't done so yet, please also see if exfat now passes the various testcases in xfstests that exercise the shutdown path.
>
> Otherwise this looks reasonable to me, thanks for the work!
>
> Hi Christoph,
>
> Thank you for your suggestion. I think the current patch is primarily aimed at addressing the issue of hotplug and ensuring writers are notified when a device has been ejected.
>
> Previously, exfat didn't have a shutdown process inherently, and hotplug didn't pose any significant issues, except for the one we're discussing in this email.
>
> Therefore, regarding what specific actions should be taken during shutdown, I would appreciate your input or any suggestions from Sungjong and Namjae.
>
> Additionally, can the shutdown handling be supplemented with another patch if there is indeed a need to implement some exfat shutdown processes?
>
> HI Sungjong and Namjae.
>
> Based on the above, what do you think, or do you have any suggestions?
There is no reason to split it into two and I would prefer to apply it
as one patch.
I would appreciate it if you could send the list an updated v4 patch
including what Christoph and I pointed out.