We can end up building up credits too slowly to do large operations
(reads and writes for example) that require many credits. By
comparison most other SMB3 clients request many more (sometimes
thousands) of credits on all operations. Increase
the number of credits we request on typical (non-large e.g
read/write) operations to 10 from 2 so we can build a pool of credits
faster.
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
We don't want to break SMB sessions if we receive signals when
sending packets through the network. Fix it by masking off signals
inside __smb_send_rqst() to avoid partial packet sends due to
interrupts.
Return -EINTR if a signal is pending and only a part of the packet
was sent. Return a success status code if the whole packet was sent
regardless of signal being pending or not. This keeps a mid entry
for the request in the pending queue and allows the demultiplex
thread to handle a response from the server properly.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
When we attempt to send a packet while the demultiplex thread
is in the middle of cifs_reconnect() we may end up returning
-ENOTSOCK to upper layers. The intent here is to retry the request
once the TCP connection is up, so change it to return -EAGAIN
instead. The latter error code is retryable and the upper layers
will retry the request if needed.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Do not allow commands other than SMB2_NEGOTIATE to be sent over
recently established TCP connections. Return -EAGAIN to let upper
layers handle it properly.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
When we have a READ lease for a file and have just issued a write
operation to the server we need to purge the cache and set oplock/lease
level to NONE to avoid reading stale data. Currently we do that
only if a write operation succedeed thus not covering cases when
a request was sent to the server but a negative error code was
returned later for some other reasons (e.g. -EIOCBQUEUED or -EINTR).
Fix this by turning off caching regardless of the error code being
returned.
The patches fixes generic tests 075 and 112 from the xfs-tests.
Cc: <stable@vger.kernel.org>
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
For kerberos mounts, the cruid is helpful to display in
/proc/mounts in order to tell which uid's krb5 cache we
got the ticket for and to tell in the multiuser krb5 case
which local users (uids) we have Kerberos authentic sessions
for.
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
When compiling with -Wformat, clang emits the following warnings:
fs/cifs/smb1ops.c:312:20: warning: format specifies type 'unsigned
short' but the argument has type 'unsigned int' [-Wformat]
tgt_total_cnt, total_in_tgt);
^~~~~~~~~~~~
fs/cifs/cifs_dfs_ref.c:289:4: warning: format specifies type 'short'
but the argument has type 'int' [-Wformat]
ref->flags, ref->server_type);
^~~~~~~~~~
fs/cifs/cifs_dfs_ref.c:289:16: warning: format specifies type 'short'
but the argument has type 'int' [-Wformat]
ref->flags, ref->server_type);
^~~~~~~~~~~~~~~~
fs/cifs/cifs_dfs_ref.c:291:4: warning: format specifies type 'short'
but the argument has type 'int' [-Wformat]
ref->ref_flag, ref->path_consumed);
^~~~~~~~~~~~~
fs/cifs/cifs_dfs_ref.c:291:19: warning: format specifies type 'short'
but the argument has type 'int' [-Wformat]
ref->ref_flag, ref->path_consumed);
^~~~~~~~~~~~~~~~~~
The types of these arguments are unconditionally defined, so this patch
updates the format character to the correct ones for ints and unsigned
ints.
Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <louis@kragniz.eu>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Adds dynamic trace points for the query_info_enter
and query_info_done (no error) case. We only had one
existing trace point related to this which was on query_info
errors. Note that these two new tracepoints are for the
non-compounded query_info paths.
Sample output (from: trace-cmd record -e smb3_query_info*)
ls-24140 [001] .... 27811.866068: smb3_query_info_enter: xid=7 sid=0xd2d00587 tid=0xb5441939 fid=0xcf082bac class=18 type=0x1
ls-24140 [001] .... 27811.867656: smb3_query_info_done: xid=7 sid=0xd2d00587 tid=0xb5441939 fid=0xcf082bac class=18 type=0x1
getcifsacl-24149 [005] .... 27854.759873: smb3_query_info_enter: xid=15 sid=0xd2d00587 tid=0xb5441939 fid=0x99896e72 class=0 type=0x3
getcifsacl-24149 [005] .... 27854.761730: smb3_query_info_done: xid=15 sid=0xd2d00587 tid=0xb5441939 fid=0x99896e72 class=0 type=0x3
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Add tracepoint before sending an SMB3 command on the wire (ie add
an smb3_cmd_enter tracepoint). This allows us to look in much
more detail at response times (between request and response).
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Add dynamic trace point for open_enter (and posix mkdir enter)
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
When ENODATA returned we weren't logging the read completion
(not an error, but can be indicated by logging length 0) which
makes looking at read traces confusing for smb3.
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Allows tracing begin (not just completion) of read, write
and query_dir which may be helpful in finding slow requests
and other timing information
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Adds two tracepoints - one for query_dir done (no err) and one for query_dir_err
Sanple output:
To start the trace in one window:
trace-cmd record -e smb3_query_dir*
Then in another window after doing an
ls /mnt
View the trace output by:
trace-cmd show
Sample output:
TASK-PID CPU# |||| TIMESTAMP FUNCTION
| | | |||| | |
ls-24869 [007] .... 90695.452009: smb3_query_dir_done: xid=7 sid=0x5027d24d tid=0xb95cf25a fid=0xc41a8c3e offset=0x0 len=0x16
ls-24869 [000] .... 90695.452764: smb3_query_dir_done: xid=8 sid=0x5027d24d tid=0xb95cf25a fid=0xc41a8c3e offset=0x0 len=0x0
ls-24874 [003] .... 90701.506342: smb3_query_dir_done: xid=11 sid=0x5027d24d tid=0xb95cf25a fid=0x33ad3601 offset=0x0 len=0x8
ls-24874 [003] .... 90701.506917: smb3_query_dir_done: xid=12 sid=0x5027d24d tid=0xb95cf25a fid=0x33ad3601 offset=0x0 len=0x0
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
POSIX negotiate context now includes the GUID specifying
which POSIX open context we support.
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Jeremy Allison <jra@samba.org>
Currently we get one credit per compound part of the request
individually. This may lead to being stuck on waiting for credits
if multiple compounded operations happen in parallel. Try acquire
credits for all compound parts at once. Return immediately if not
enough credits and too few requests are in flight currently thus
narrowing the possibility of infinite waiting for credits.
The more advance fix is to return right away if not enough credits
for the compound request and do not look at the number of requests
in flight. The caller should handle such situations by falling back
to sequential execution of SMB commands instead of compounding.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Now we just return NULL cifsFileInfo pointer in cases we didn't find
or couldn't reopen a file. This hides errors from cifs_reopen_file()
especially retryable errors which should be handled appropriately.
Create new cifs_get_writable_file() routine that returns error codes
from cifs_reopen_file() and use it in the writeback codepath.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we check for an open file existence in wdata_send_pages()
which doesn't provide an easy way to handle error codes that will
be returned from find_writable_filehandle() once it is changed.
Move the check to writepages.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently wdata_send_pages() unlocks pages after sending.
This complicates further refactoring and doesn't align
with the function name. Move unlocking to writepages.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reorder finding and reopening a writable handle file and getting
MTU credits in writepages because we may be stuck on low credits
otherwise.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we get MTU credits before we check an open file if
it needs to be reopened. Reopening the file in such conditions
leads to a possibility of being stuck waiting indefinitely
for credits in the transport layer. Fix this by reopening the
file first if needed and then getting MTU credits for async IO.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we do proper accounting for credits in regards to
reconnects and error handling, thus we do not need custom
credit adjustments when reconnect is detected developed
previously.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we adjust MTU credits before sending an IO request
and after reopening a file. This approach doesn't allow the
reopen routine to use existing credits that are not needed
for IO. Reorder credit adjustment and reopening a file to
use credits available to the client more efficiently. Also
unwrap complex if statement into few pieces to improve
readability.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
The reconnect might have happended after we obtained credits
and before we acquired srv_mutex. Check for that under the mutex
and retry a sync operation if the reconnect is detected.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
The reconnect might have happended after we obtained credits
and before we acquired srv_mutex. Check for that under the mutex
and retry an async operation if the reconnect is detected.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Every time after a session reconnect we don't need to account for
credits obtained in previous sessions. Make use of the recently
added cifs_credits structure to properly calculate credits for
non-MTU requests the same way we did for MTU ones.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Every time after a session reconnect we don't need to account for
credits obtained in previous sessions. Introduce new struct cifs_credits
which contains both credits value and reconnect instance of the
time those credits were taken. Modify a routine that add credits
back to handle the reconnect instance by assuming zero credits
if the reconnect happened after the credits were obtained and
before we decided to add them back due to some errors during sending.
This patch fixes the MTU credits cases. The subsequent patch
will handle non-MTU ones.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we set reconnect instance to zero on the first
connection but this is not convenient because we need to
reserve some special value for credit handling on reconnects
which is coming in subsequent patches. Fix this by starting
with one when initiating a new TCP connection.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
There are a couple places where we still account for 4 bytes
in the beginning of SMB2 packet which is not true in the current
code. Fix this to use a header preamble size where possible.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Even if a response is malformed, we should count credits
granted by the server to avoid miscalculations and unnecessary
reconnects due to client or server bugs. If the response has
been received partially, the session will be reconnected anyway
on the next iteration of the demultiplex thread, so counting
credits for such cases shouldn't break things.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we only skip credits logging on reconnects. When
unmounting a share the number of credits on the client doesn't
matter, so skip logging in such cases too.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Currently we skip setting a read error to -EIO if a stored
result is -ENODATA and a response hasn't been received. With
the recent changes in read error processing there shouldn't be
cases when -ENODATA is set without a response from the server,
so reset the error to -EIO unconditionally.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
When we hit failures during constructing MIDs or sending PDUs
through the network, we end up not using message IDs assigned
to the packet. The next SMB packet will skip those message IDs
and continue with the next one. This behavior may lead to a server
not granting us credits until we use the skipped IDs. Fix this by
reverting the current ID to the original value if any errors occur
before we push the packet through the network stack.
This patch fixes the generic/310 test from the xfs-tests.
Cc: <stable@vger.kernel.org> # 4.19.x
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
If we try large I/O (read or write) immediately after mount
we won't typically have enough credits because we only request
large amounts of credits on the first session setup. So if
large I/O is attempted soon after mount we will typically only
have about 43 credits rather than 105 credits (with this patch)
available for the large i/o (which needs 64 credits minimum).
This patch requests more credits during tree connect, which
helps ensure that we have enough credits when mount completes
(between these requests and the first session setup) in order
to start large I/O immediately after mount if needed.
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
We negotiate rsize mounts (and it can be overridden by user) to
typically 4MB, so using larger default I/O sizes from userspace
(changing to 1MB default i/o size returned by stat) the
performance is much better (and not just for long latency
network connections) in most use cases for SMB3 than the default I/O
size (which ends up being 128K for cp and can be even smaller for cp).
This can be 4x slower or worse depending on network latency.
By changing inode->blocksize from 32K (which was perhaps ok
for very old SMB1/CIFS) to a larger value, 1MB (but still less than
max size negotiated with the server which is 4MB, in order to minimize
risk) it significantly increases performance for the
noncached case, and slightly increases it for the cached case.
This can be changed by the user on mount (specifying bsize=
values from 16K to 16MB) to tune better for performance
for applications that depend on blocksize.
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
CC: Stable <stable@vger.kernel.org>
Currently on lease break the client sets a caching level twice:
when oplock is detected and when oplock is processed. While the
1st attempt sets the level to the value provided by the server,
the 2nd one resets the level to None unconditionally.
This happens because the oplock/lease processing code was changed
to avoid races between page cache flushes and oplock breaks.
The commit c11f1df500 ("cifs: Wait for writebacks to complete
before attempting write.") fixed the races for oplocks but didn't
apply the same changes for leases resulting in overwriting the
server granted value to None. Fix this by properly processing
lease breaks.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
CC: Stable <stable@vger.kernel.org>
/proc/fs/cifs/Stats bytes_read was double counting reads when
uncached (ie mounted with cache=none)
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
BUGZILLA: https://bugzilla.kernel.org/show_bug.cgi?id=202007
When deleting an xattr/EA:
SMB2/3 servers will return SUCCESS when clients delete non-existing EAs.
This means that we need to first QUERY the server and check if the EA
exists or not so that we can return -ENODATA correctly when this happens.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
We should add any credits granted to us from unmatched server responses.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
a trivial patch that replaces all use of snprintf with scnprintf.
scnprintf() is generally seen as a safer function to use than
snprintf for many use cases.
In our case, there is no actual difference between the two since we never
look at the return value. Thus we did not have any of the bugs that
scnprintf protects against and the patch does nothing.
However, for people reading our code it will be a receipt that we
have done our due dilligence and checked our code for this type of bugs.
See the presentation "Making C Less Dangerous In The Linux Kernel"
at this years LCA
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
There is a NULL pointer dereference of devname in strspn()
The oops looks something like:
CIFS: Attempting to mount (null)
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
...
RIP: 0010:strspn+0x0/0x50
...
Call Trace:
? cifs_parse_mount_options+0x222/0x1710 [cifs]
? cifs_get_volume_info+0x2f/0x80 [cifs]
cifs_setup_volume_info+0x20/0x190 [cifs]
cifs_get_volume_info+0x50/0x80 [cifs]
cifs_smb3_do_mount+0x59/0x630 [cifs]
? ida_alloc_range+0x34b/0x3d0
cifs_do_mount+0x11/0x20 [cifs]
mount_fs+0x52/0x170
vfs_kern_mount+0x6b/0x170
do_mount+0x216/0xdc0
ksys_mount+0x83/0xd0
__x64_sys_mount+0x25/0x30
do_syscall_64+0x65/0x220
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Fix this by adding a NULL check on devname in cifs_parse_devname()
Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
Signed-off-by: Steve French <stfrench@microsoft.com>
If we don't find a writable file handle when retrying writepages
we break of the loop and do not unlock and put pages neither from
wdata2 nor from the original wdata. Fix this by walking through
all the remaining pages and cleanup them properly.
Cc: <stable@vger.kernel.org>
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
The request buffers are freed right before copying the pointers.
Use the func args instead which are identical and still valid.
Simple reproducer (requires KASAN enabled) on a cifs mount:
echo foo > foo ; tail -f foo & rm foo
Cc: <stable@vger.kernel.org> # 4.20
Fixes: 179e44d49c ("smb3: add tracepoint for sending lease break responses to server")
Signed-off-by: Aurelien Aptel <aaptel@suse.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
When doing reads beyound the end of a file the server returns
error STATUS_END_OF_FILE error which is mapped to -ENODATA.
Currently we report it as a failure which confuses read stats.
Change it to not consider -ENODATA as failure for stat purposes.
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
CC: Stable <stable@vger.kernel.org>
Currently we log success once we send an async IO request to
the server. Instead we need to analyse a response and then log
success or failure for a particular command. Also fix argument
list for read logging.
Cc: <stable@vger.kernel.org> # 4.18
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Allocation of a page array for non-cached IO was separated from
allocation of rdata and wdata structures and this introduced memory
leaks and a possible null pointer dereference. This patch fixes
these problems.
Cc: <stable@vger.kernel.org>
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
minus the various headers and blobs that will be part of the reply.
or else we might trigger a session reconnect.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Steve French