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

Huiwen He <[email protected]> writes:

> From: Huiwen He <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: ChenXiaoSong <[email protected]>
> ---
>  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.