[PATCH] smb/client: fix unaligned fallocate emulation with O_DIRECT

[PATCH] smb/client: fix unaligned fallocate emulation with O_DIRECT

[Huiwen He]

From: Huiwen He <hehuiwen@kylinos.cn>

Commit 4a7d2729dc99 ("smb: client: fix atomic open with O_DIRECT & O_SYNC")
made atomic O_DIRECT opens correctly use CREATE_NO_BUFFER. This exposed
an issue in the fallocate emulation added by
commit 966a3cb7c7db ("cifs: improve fallocate emulation"), which may
reuse the handle for unaligned zero writes.

Windows requires the offset and length of writes on an
unbuffered handle to be sector aligned, and rejects these writes
with STATUS_INVALID_PARAMETER, causing xfstests generic/760 to fail.

For SMB3.02 and later, fix this by keeping the handle buffered and using
READ_UNBUFFERED or WRITE_UNBUFFERED only for actual direct I/O requests.
Determine this from the netfs request origin rather than the file flags,
since mmap writeback on an O_DIRECT file is still buffered.

For older dialects, retain CREATE_NO_BUFFER since they do not support
per-I/O unbuffered flags.

With this change, xfstests generic/760 passes against Windows with SMB3.02
or later, while Windows SMB3.0 remains affected.

Fixes: 4a7d2729dc99 ("smb: client: fix atomic open with O_DIRECT & O_SYNC")
Signed-off-by: Huiwen He <hehuiwen@kylinos.cn>
Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
---
 fs/smb/client/cifsglob.h | 13 +++++++++++--
 fs/smb/client/dir.c      |  2 +-
 fs/smb/client/file.c     |  4 ++--
 fs/smb/client/smb2pdu.c  |  6 ++++++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 08e94633a9c1..2e43dbd564de 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -2374,17 +2374,26 @@ static inline void cifs_reset_oplock(struct cifsInodeInfo *cinode)
 		WRITE_ONCE(cinode->oplock, 0);
 }
 
+static inline bool
+cifs_server_supports_per_io_unbuffered(const struct TCP_Server_Info *server)
+{
+	return server && server->dialect >= SMB302_PROT_ID;
+}
+
 static inline bool cifs_forced_shutdown(const struct cifs_sb_info *sbi)
 {
 	return cifs_sb_flags(sbi) & CIFS_MOUNT_SHUTDOWN;
 }
 
-static inline int cifs_open_create_options(unsigned int oflags, int opts)
+static inline int
+cifs_open_create_options(const struct TCP_Server_Info *server,
+			 unsigned int oflags, int opts)
 {
 	/* O_SYNC also has bit for O_DSYNC so following check picks up either */
 	if (oflags & O_SYNC)
 		opts |= CREATE_WRITE_THROUGH;
-	if (oflags & O_DIRECT)
+	if ((oflags & O_DIRECT) &&
+	    !cifs_server_supports_per_io_unbuffered(server))
 		opts |= CREATE_NO_BUFFER;
 	if (oflags & O_TMPFILE)
 		opts |= CREATE_DELETE_ON_CLOSE;
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 88a4a1787ff0..72b1b13c560e 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -336,7 +336,7 @@ static int __cifs_do_create(struct inode *dir, struct dentry *direntry,
 	if (!server->ops->open)
 		return -EOPNOTSUPP;
 
-	create_options |= cifs_open_create_options(oflags, create_options);
+	create_options |= cifs_open_create_options(server, oflags, create_options);
 	/*
 	 * if we're not using unix extensions, see if we need to set
 	 * ATTR_READONLY on the create call
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 968740e7c9c3..d4d23a313869 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -597,7 +597,7 @@ static int cifs_nt_open(const char *full_path, struct inode *inode, struct cifs_
 
 	disposition = cifs_get_disposition(f_flags);
 	/* BB pass O_SYNC flag through on file attributes .. BB */
-	create_options |= cifs_open_create_options(f_flags, create_options);
+	create_options |= cifs_open_create_options(server, f_flags, create_options);
 
 retry_open:
 	oparms = (struct cifs_open_parms) {
@@ -1321,7 +1321,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
 		rdwr_for_fscache = 1;
 
 	desired_access = cifs_convert_flags(cfile->f_flags, rdwr_for_fscache);
-	create_options |= cifs_open_create_options(cfile->f_flags,
+	create_options |= cifs_open_create_options(server, cfile->f_flags,
 						   create_options);
 
 	if (server->ops->get_lease_key)
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 4ce165e40657..bbbcb5b9bdf2 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -4578,6 +4578,9 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
 	req->MinimumCount = 0;
 	req->Length = cpu_to_le32(io_parms->length);
 	req->Offset = cpu_to_le64(io_parms->offset);
+	if (rdata && rdata->rreq->origin == NETFS_DIO_READ &&
+	    cifs_server_supports_per_io_unbuffered(server))
+		req->Flags = SMB2_READFLAG_READ_UNBUFFERED;
 
 	trace_smb3_read_enter(rdata ? rdata->rreq->debug_id : 0,
 			      rdata ? rdata->subreq.debug_index : 0,
@@ -5170,6 +5173,9 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
 	req->DataOffset = cpu_to_le16(
 				offsetof(struct smb2_write_req, Buffer));
 	req->RemainingBytes = 0;
+	if (wdata->rreq->origin == NETFS_DIO_WRITE &&
+	    cifs_server_supports_per_io_unbuffered(server))
+		req->Flags = cpu_to_le32(SMB2_WRITEFLAG_WRITE_UNBUFFERED);
 
 	trace_smb3_write_enter(wdata->rreq->debug_id,
 			       wdata->subreq.debug_index,
-- 
2.43.0

Re: [PATCH] smb/client: fix unaligned fallocate emulation with O_DIRECT

[Paulo Alcantara]

Huiwen He <huiwen.he@linux.dev> writes:

> From: Huiwen He <hehuiwen@kylinos.cn>
>
> Commit 4a7d2729dc99 ("smb: client: fix atomic open with O_DIRECT & O_SYNC")
> made atomic O_DIRECT opens correctly use CREATE_NO_BUFFER. This exposed
> an issue in the fallocate emulation added by
> commit 966a3cb7c7db ("cifs: improve fallocate emulation"), which may
> reuse the handle for unaligned zero writes.
>
> Windows requires the offset and length of writes on an
> unbuffered handle to be sector aligned, and rejects these writes
> with STATUS_INVALID_PARAMETER, causing xfstests generic/760 to fail.
>
> For SMB3.02 and later, fix this by keeping the handle buffered and using
> READ_UNBUFFERED or WRITE_UNBUFFERED only for actual direct I/O requests.
> Determine this from the netfs request origin rather than the file flags,
> since mmap writeback on an O_DIRECT file is still buffered.
>
> For older dialects, retain CREATE_NO_BUFFER since they do not support
> per-I/O unbuffered flags.
>
> With this change, xfstests generic/760 passes against Windows with SMB3.02
> or later, while Windows SMB3.0 remains affected.
>
> Fixes: 4a7d2729dc99 ("smb: client: fix atomic open with O_DIRECT & O_SYNC")
> Signed-off-by: Huiwen He <hehuiwen@kylinos.cn>
> Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
> ---
>  fs/smb/client/cifsglob.h | 13 +++++++++++--
>  fs/smb/client/dir.c      |  2 +-
>  fs/smb/client/file.c     |  4 ++--
>  fs/smb/client/smb2pdu.c  |  6 ++++++
>  4 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 08e94633a9c1..2e43dbd564de 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -2374,17 +2374,26 @@ static inline void cifs_reset_oplock(struct cifsInodeInfo *cinode)
>  		WRITE_ONCE(cinode->oplock, 0);
>  }
>  
> +static inline bool
> +cifs_server_supports_per_io_unbuffered(const struct TCP_Server_Info *server)
> +{
> +	return server && server->dialect >= SMB302_PROT_ID;
> +}
> +

Unfortunately this check is unsufficient to tell whether the server will
actually support SMB2_WRITEFLAG_WRITE_UNBUFFERED and
SMB2_READFLAG_READ_UNBUFFERED flags.

Have you tested this against Azure server?  IIRC, the server would
reject the write and read requests with STATUS_INVALID_PARAMETER if any
of those flags was set.

I had a patch to use those flags and improve reuse of open handles but
then gave up as Azure server seems to be the only one broken regarding
those flags.

Re: [PATCH] smb/client: fix unaligned fallocate emulation with O_DIRECT

[hehuiwen]

Thanks for pointing this out. I tested the patch against Windows Server,
Samba and ksmbd, but not Azure Files.

I will look into this further.

Thanks,

Huiwen


在 2026/7/16 23:09, Paulo Alcantara 写道:
> Huiwen He <huiwen.he@linux.dev> writes:
> 
>> From: Huiwen He <hehuiwen@kylinos.cn>
>>
>> Commit 4a7d2729dc99 ("smb: client: fix atomic open with O_DIRECT & O_SYNC")
>> made atomic O_DIRECT opens correctly use CREATE_NO_BUFFER. This exposed
>> an issue in the fallocate emulation added by
>> commit 966a3cb7c7db ("cifs: improve fallocate emulation"), which may
>> reuse the handle for unaligned zero writes.
>>
>> Windows requires the offset and length of writes on an
>> unbuffered handle to be sector aligned, and rejects these writes
>> with STATUS_INVALID_PARAMETER, causing xfstests generic/760 to fail.
>>
>> For SMB3.02 and later, fix this by keeping the handle buffered and using
>> READ_UNBUFFERED or WRITE_UNBUFFERED only for actual direct I/O requests.
>> Determine this from the netfs request origin rather than the file flags,
>> since mmap writeback on an O_DIRECT file is still buffered.
>>
>> For older dialects, retain CREATE_NO_BUFFER since they do not support
>> per-I/O unbuffered flags.
>>
>> With this change, xfstests generic/760 passes against Windows with SMB3.02
>> or later, while Windows SMB3.0 remains affected.
>>
>> Fixes: 4a7d2729dc99 ("smb: client: fix atomic open with O_DIRECT & O_SYNC")
>> Signed-off-by: Huiwen He <hehuiwen@kylinos.cn>
>> Reviewed-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
>> ---
>>   fs/smb/client/cifsglob.h | 13 +++++++++++--
>>   fs/smb/client/dir.c      |  2 +-
>>   fs/smb/client/file.c     |  4 ++--
>>   fs/smb/client/smb2pdu.c  |  6 ++++++
>>   4 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> index 08e94633a9c1..2e43dbd564de 100644
>> --- a/fs/smb/client/cifsglob.h
>> +++ b/fs/smb/client/cifsglob.h
>> @@ -2374,17 +2374,26 @@ static inline void cifs_reset_oplock(struct cifsInodeInfo *cinode)
>>   		WRITE_ONCE(cinode->oplock, 0);
>>   }
>>   
>> +static inline bool
>> +cifs_server_supports_per_io_unbuffered(const struct TCP_Server_Info *server)
>> +{
>> +	return server && server->dialect >= SMB302_PROT_ID;
>> +}
>> +
> 
> Unfortunately this check is unsufficient to tell whether the server will
> actually support SMB2_WRITEFLAG_WRITE_UNBUFFERED and
> SMB2_READFLAG_READ_UNBUFFERED flags.
> 
> Have you tested this against Azure server?  IIRC, the server would
> reject the write and read requests with STATUS_INVALID_PARAMETER if any
> of those flags was set.
> 
> I had a patch to use those flags and improve reuse of open handles but
> then gave up as Azure server seems to be the only one broken regarding
> those flags.