Re: Additional performance data on Pavel's smb3 multi credit patch series

[ronnie sahlberg]

Impressive!

On Sun, Jul 13, 2014 at 12:23 PM, Steve French smfre...@gmail.com wrote:
 Performance of Pavel's multicredit i/o SMB3 patches continues to look
 good.  Additional informal performance results below comparing cifs
 mounts with smb3 mounts (vers=3.0) with and without Pavel's patch set.
 I plan to do additional testing with large rsize/wsize (default with
 Pavel's code is 1MB).

 3.16-rc4 (Ubuntu) on client.  Server is Windows 8.1.  Both VMs on same
 host (host disk is fairly fast SSD).

 Copy to server performance increased about 20% percent
 dd if=/dev/zero of=/mnt/targetfile bs=80M count=25
 got similar results with or without conv=fdatasync

 1st run copying to empty directory, 2nd run copying over targetfile,
 (pattern repeated multiple times) averaging results

 New code (with Pavel's patches)
 -
 CIFS 167MB/s
 SMB3 200MB/s

 Existing code (without his patches)
 
 SMB3 166MB/s
 CIFS 164.5MB/s

 For large file reading SMB3 performance with Pavel's patches increased
 76% over existing SMB3 code
 dd of=/dev/null if=/mnt/targetfile bs=80M count=25
 (mounting and unmounting between attempts to avoid caching effects on
 the client)

 New code (with Pavel's patches)
 -
 CIFS 114MB/s
 SMB3 216MB/s

 Existing code (without his patches)
 
 SMB3 123MB/s
 CIFS 110MB/s

 --
 Thanks,

 Steve
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Patch v7 05/22] CIFS: SMBD: Establish SMB Direct connection

[ronnie sahlberg]

On Tue, Nov 7, 2017 at 6:54 PM, Long Li  wrote:
> From: Long Li 
>
> Add code to implement the core functions to establish a SMB Direct connection.
>
> 1. Establish an RDMA connection to SMB server.
> 2. Negotiate and setup SMB Direct protocol.
> 3. Implement idle connection timer and credit management.
>
> SMB Direct is enabled by setting CONFIG_CIFS_SMB_DIRECT.
>
> Add to Makefile to enable building SMB Direct.
>
> Signed-off-by: Long Li 
> ---
>  fs/cifs/Makefile|2 +
>  fs/cifs/smbdirect.c | 1576 
> +++
>  fs/cifs/smbdirect.h |  280 +
>  3 files changed, 1858 insertions(+)
>
> diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile
> index 5e853a3..ad00873 100644
> --- a/fs/cifs/Makefile
> +++ b/fs/cifs/Makefile
> @@ -18,3 +18,5 @@ cifs-$(CONFIG_CIFS_UPCALL) += cifs_spnego.o
>  cifs-$(CONFIG_CIFS_DFS_UPCALL) += dns_resolve.o cifs_dfs_ref.o
>
>  cifs-$(CONFIG_CIFS_FSCACHE) += fscache.o cache.o
> +
> +cifs-$(CONFIG_CIFS_SMB_DIRECT) += smbdirect.o
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index d3c16f8..021d527 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -13,7 +13,34 @@
>   *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>   *   the GNU General Public License for more details.
>   */
> +#include 
>  #include "smbdirect.h"
> +#include "cifs_debug.h"
> +
> +static struct smbd_response *get_empty_queue_buffer(
> +   struct smbd_connection *info);
> +static struct smbd_response *get_receive_buffer(
> +   struct smbd_connection *info);
> +static void put_receive_buffer(
> +   struct smbd_connection *info,
> +   struct smbd_response *response,
> +   bool lock);
> +static int allocate_receive_buffers(struct smbd_connection *info, int 
> num_buf);
> +static void destroy_receive_buffers(struct smbd_connection *info);
> +
> +static void put_empty_packet(
> +   struct smbd_connection *info, struct smbd_response *response);
> +static void enqueue_reassembly(
> +   struct smbd_connection *info,
> +   struct smbd_response *response, int data_length);
> +static struct smbd_response *_get_first_reassembly(
> +   struct smbd_connection *info);
> +
> +static int smbd_post_recv(
> +   struct smbd_connection *info,
> +   struct smbd_response *response);
> +
> +static int smbd_post_send_empty(struct smbd_connection *info);
>
>  /* SMBD version number */
>  #define SMBD_V10x0100
> @@ -75,3 +102,1552 @@ int smbd_max_frmr_depth = 2048;
>
>  /* If payload is less than this byte, use RDMA send/recv not read/write */
>  int rdma_readwrite_threshold = 4096;
> +
> +/* Transport logging functions
> + * Logging are defined as classes. They can be OR'ed to define the actual
> + * logging level via module parameter smbd_logging_class
> + * e.g. cifs.smbd_logging_class=0x500 will log all log_rdma_recv() and
> + * log_rdma_event()

Should be 0xa0 ?

> + */
> +#define LOG_OUTGOING   0x1
> +#define LOG_INCOMING   0x2
> +#define LOG_READ   0x4
> +#define LOG_WRITE  0x8
> +#define LOG_RDMA_SEND  0x10
> +#define LOG_RDMA_RECV  0x20
> +#define LOG_KEEP_ALIVE 0x40
> +#define LOG_RDMA_EVENT 0x80
> +#define LOG_RDMA_MR0x100
> +static unsigned int smbd_logging_class = 0;
> +module_param(smbd_logging_class, uint, 0644);
> +MODULE_PARM_DESC(smbd_logging_class,
> +   "Logging class for SMBD transport 0x0 to 0x100");
> +
> +#define ERR0x0
> +#define INFO   0x1
> +static unsigned int smbd_logging_level = ERR;
> +module_param(smbd_logging_level, uint, 0644);
> +MODULE_PARM_DESC(smbd_logging_level,
> +   "Logging level for SMBD transport, 0 (default): error, 1: info");
> +
> +#define log_rdma(level, class, fmt, args...)   \
> +do {   \
> +   if (level <= smbd_logging_level || class & smbd_logging_class)  \
> +   cifs_dbg(VFS, "%s:%d " fmt, __func__, __LINE__, ##args);\
> +} while (0)
> +
> +#define log_outgoing(level, fmt, args...) \
> +   log_rdma(level, LOG_OUTGOING, fmt, ##args)
> +#define log_incoming(level, fmt, args...) \
> +   log_rdma(level, LOG_INCOMING, fmt, ##args)
> +#define log_read(level, fmt, args...)  log_rdma(level, LOG_READ, fmt, ##args)
> +#define log_write(level, fmt, args...) log_rdma(level, LOG_WRITE, fmt, 
> ##args)
> +#define log_rdma_send(level, fmt, args...) \
> +   log_rdma(level, LOG_RDMA_SEND, fmt, ##args)
> +#define log_rdma_recv(level, fmt, args...) \
> +   log_rdma(level, LOG_RDMA_RECV, fmt, ##args)
> +#define log_keep_alive(level, fmt, args...) \
> +   

Re: [PATCHv2][SMB3] Add kernel trace support

[Ronnie Sahlberg]

Very nice.

Acked-by: Ronnie Sahlberg <lsahl...@redhat.com>

Possibly change the output from 
pid=6633 tid=0x0 sid=0x0 cmd=0 mid=0
to
cmd=0 mid=0 pid=6633 tid=0x0 sid=0x0

just to make it easier for human-searching. I think the cmd will be useful much 
more often than pid/tid/sid
and this would make it easier to look for as all cmd= entries will be aligned 
to the same column.



- Original Message -
From: "Steve French" <smfre...@gmail.com>
To: "CIFS" <linux-c...@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, 
"samba-technical" <samba-techni...@lists.samba.org>, "linux-fsdevel" 
<linux-fsde...@vger.kernel.org>
Sent: Friday, 18 May, 2018 12:36:36 PM
Subject: [PATCHv2][SMB3] Add kernel trace support

Patch updated with additional tracepoint locations and some formatting
improvements. There are some obvious additional tracepoints that could
be added, but this should be a reasonable group to start with.

>From edc02d6f9dc24963d510c7ef59067428d3b082d3 Mon Sep 17 00:00:00 2001
From: Steve French <stfre...@microsoft.com>
Date: Thu, 17 May 2018 21:16:55 -0500
Subject: [PATCH] smb3: Add ftrace tracepoints for improved SMB3 debugging

Although dmesg logs and wireshark network traces can be
helpful, being able to dynamically enable/disable tracepoints
(in this case via the kernel ftrace mechanism) can also be
helpful in more quickly debugging problems, and more
selectively tracing the events related to the bug report.

This patch adds 12 ftrace tracepoints to cifs.ko for SMB3 events
in some obvious locations.  Subsequent patches will add more
as needed.

Example use:
   trace-cmd record -e cifs
   
   trace-cmd show

Various trace events can be filtered. See:
   trace-cmd list | grep cifs
for the current list of cifs tracepoints.

Sample output (from mount and writing to a file):

root@smf:/sys/kernel/debug/tracing/events/cifs# trace-cmd show

  mount.cifs-6633  [006]   7246.936461: smb3_cmd_done:
pid=6633 tid=0x0 sid=0x0 cmd=0 mid=0
  mount.cifs-6633  [006]   7246.936701: smb3_cmd_err:
pid=6633 tid=0x0 sid=0x3d9cf8e5 cmd=1 mid=1 status=0xc016 rc=-5
  mount.cifs-6633  [006]   7246.943055: smb3_cmd_done:
pid=6633 tid=0x0 sid=0x3d9cf8e5 cmd=1 mid=2
  mount.cifs-6633  [006]   7246.943298: smb3_cmd_done:
pid=6633 tid=0xf9447636 sid=0x3d9cf8e5 cmd=3 mid=3
  mount.cifs-6633  [006]   7246.943446: smb3_cmd_done:
pid=6633 tid=0xf9447636 sid=0x3d9cf8e5 cmd=11 mid=4
  mount.cifs-6633  [006]   7246.943659: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=3 mid=5
  mount.cifs-6633  [006]   7246.943766: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=11 mid=6
  mount.cifs-6633  [006]   7246.943937: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=5 mid=7
  mount.cifs-6633  [006]   7246.944020: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=8
  mount.cifs-6633  [006]   7246.944091: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=9
  mount.cifs-6633  [006]   7246.944163: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=10
  mount.cifs-6633  [006]   7246.944218: smb3_cmd_err:
pid=6633 tid=0xf9447636 sid=0x3d9cf8e5 cmd=11 mid=11 status=0xc225
rc=-2
  mount.cifs-6633  [006]   7246.944219: smb3_fsctl_err: xid=0
fid=0x tid=0xf9447636 sid=0x3d9cf8e5 class=0
type=393620 rc=-2
  mount.cifs-6633  [007]   7246.944353: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=12
bash-2071  [000]   7256.903844: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=5 mid=13
bash-2071  [000]   7256.904172: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=14
bash-2071  [000]   7256.904471: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=17 mid=15
bash-2071  [000]   7256.904950: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=5 mid=16
bash-2071  [000]   7256.905305: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=17 mid=17
bash-2071  [000]   7256.905688: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=6 mid=18
bash-2071  [000]   7256.905809: smb3_write_done: xid=0
fid=0xd628f511 tid=0xe1b781a sid=0x3d9cf8e5 offset=0x0 len=0x1b

Signed-off-by: Steve French <stfre...@microsoft.com>
---
 fs/cifs/Makefile   |   7 +-
 fs/cifs/smb2maperror.c |  10 +-
 fs/cifs/smb2pdu.c  |  56 +++-
 fs/cifs/trace.c|  18 +++
 fs/cifs/trace.h| 298 +
 5 files changed, 379 insertions(+), 10 deletions(-)
 create mode 100644 fs/cifs/trace.c
 create mode 100644 fs/cifs/trace.h

diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile
index 7e4a1e2f0696..85817991ee68 100644
--- a/fs/cifs/Makefile
+++ b/fs/cifs/Makefile
@@ -1

Re: [PATCHv2][SMB3] Add kernel trace support

[ronnie sahlberg]

Fair enough.

Onto a second point then, for future patches.

There are a lot of places where we do not (yet) pass a tcon as argument.
Can we make a policy decision to mandate that every tracepoint MUST
log tid, sid?

And thus, if you need a new tracepoint in a function where tcon is not
available you then first
have to do the plumbing to pass the tcon all the way down to the tracepoint?


Ideally we should pass tcon into any and every leaf function for
tracepoints as well as debug logging.


On Fri, May 18, 2018 at 4:19 PM, Steve French <smfre...@gmail.com> wrote:
> On Thu, May 17, 2018 at 10:28 PM, Ronnie Sahlberg <lsahl...@redhat.com> wrote:
>> Very nice.
>>
>> Acked-by: Ronnie Sahlberg <lsahl...@redhat.com>
>>
>> Possibly change the output from
>> pid=6633 tid=0x0 sid=0x0 cmd=0 mid=0
>> to
>> cmd=0 mid=0 pid=6633 tid=0x0 sid=0x0
>>
>> just to make it easier for human-searching. I think the cmd will be useful 
>> much more often than pid/tid/sid
>> and this would make it easier to look for as all cmd= entries will be 
>> aligned to the same column.
>
> My instinct is to preserve the consistency by beginning with the the
> fields that will be in 90% of the commands: tree id and session id
> (tid and sid), which would cause pid to move after sid or after cmd,
> but I would prefer to wait on reordering fields and fixing alignment
> till we add another set of tracepoints (e.g. in FreeXid, and in
> SMB2_open and in the caller of negprot/sessionsetup) - we should then
> have a better idea what formatting would make it slightly more
> consistent and readable.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linux-next: manual merge of the cifs tree with Linus' tree

[Ronnie Sahlberg]

Steve,

Looks good. It builds and seems to work in my testing.
Thanks.

- Original Message -
From: "Steve French" 
To: "Stephen Rothwell" 
Cc: "CIFS" , "Linux-Next Mailing List" 
, "Linux Kernel Mailing List" 
, "Kees Cook" , "Ronnie 
Sahlberg" , "Aurelien Aptel" 
Sent: Thursday, 14 June, 2018 10:40:43 AM
Subject: Re: linux-next: manual merge of the cifs tree with Linus' tree

Ronnie,

I fixed up that patch and repushed to cifs-2.6.git for-next.  Seems
trivial. Let me know if any issues.

On Wed, Jun 13, 2018 at 6:01 PM, Stephen Rothwell  wrote:
> Hi all,
>
> Today's linux-next merge of the cifs tree got a conflict in:
>
>   fs/cifs/transport.c
>
> between commit:
>
>   6da2ec56059c ("treewide: kmalloc() -> kmalloc_array()")
>
> from Linus' tree and commit:
>
>   1560d69e21c6 ("cifs: push rfc1002 generation down the stack")
>
> from the cifs tree.
>
> I fixed it up (the latter removed the code modified by the former) and
> can carry the fix as necessary. This is now fixed as far as linux-next
> is concerned, but any non trivial conflicts should be mentioned to your
> upstream maintainer when your tree is submitted for merging.  You may
> also want to consider cooperating with the maintainer of the conflicting
> tree to minimise any particularly complex conflicts.
>
> --
> Cheers,
> Stephen Rothwell



-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: F_OFD_GETLK implemented wrong with CIFS protocol version 2.0+

[Ronnie Sahlberg]

The problem is in fs/cifs/file.c:cifs_find_fid_lock_conflict since it is not 
aware of OFD locks
and thus think there is a conflict.

I have an initial patch that fixes the problem for the reproducer but need more 
time to understand if/what
else might need fixin.





- Original Message -
> From: "Steve French" 
> To: labb...@redhat.com
> Cc: "CIFS" , "samba-technical" 
> , "LKML"
> , "Adam Williamson" 
> Sent: Tuesday, 26 June, 2018 1:54:40 PM
> Subject: Re: F_OFD_GETLK implemented wrong with CIFS protocol version 2.0+
> 
> We are taking a look at this - Ronnie had some ideas.  Probably simply
> not implemented - hopefully not too hard to fix.
> On Mon, Jun 25, 2018 at 6:58 PM Laura Abbott  wrote:
> >
> > Hi,
> >
> > A while back, someone reported a failure on Fedora when trying to boot
> > a QEMU image off of a CIFS share. The issue was reduced down to a
> > test case (https://bugzilla.redhat.com/show_bug.cgi?id=1484130#c8)
> >
> > # cat test-ofd-lock.c
> > #define _GNU_SOURCE
> > #include 
> > #include 
> > #include 
> > #include 
> >
> > int main(int argc, char **argv)
> > {
> >  int ret;
> >  int fd;
> >  struct flock fl = {
> >  .l_whence = SEEK_SET,
> >  .l_start  = 0,
> >  .l_len= 0,
> >  .l_type   = F_RDLCK,
> >  };
> >  if (argc < 2) {
> >  fprintf(stderr, "Usage: %s \n", argv[0]);
> >  return 1;
> >  }
> >  fd = open(argv[1], O_RDWR);
> >  if (fd < 0) {
> >  perror("open");
> >  return errno;
> >  }
> >  ret = fcntl(fd, F_OFD_SETLK, );
> >  if (ret) {
> >  perror("setlk");
> >  return errno;
> >  }
> >  fl.l_type = F_WRLCK;
> >  ret = fcntl(fd, F_OFD_GETLK, );
> >  if (ret) {
> >  perror("getlk");
> >  return errno;
> >  }
> >  if (fl.l_type != F_UNLCK) {
> >  fprintf(stderr, "get lock test failed\n");
> >  return 1;
> >  }
> >  return 0;
> > }
> > [root@localhost ~]# make test-ofd-lock
> > cc test-ofd-lock.c   -o test-ofd-lock
> > [root@localhost ~]# touch /tmp/test && ./test-ofd-lock /tmp/test
> > [root@localhost ~]# echo $?
> > 0
> > [root@localhost ~]# touch /mnt/test && ./test-ofd-lock /mnt/test
> > get lock test failed
> > [root@localhost ~]# mount | grep /mnt
> > //192.168.31.1/tddownload on /mnt type cifs (rw,relatime,vers=3.0,
> > cache=strict,username=admin,domain=,uid=0,
> > noforceuid,gid=0,noforcegid,addr=192.168.31.1,file_mode=0755,
> > dir_mode=0755,nounix,serverino,mapposix,rsize=1048576,
> > wsize=1048576,echo_interval=60,actimeo=1,user=admin)
> >
> >
> > As explained by one of the QEMU developers
> > (https://bugzilla.redhat.com/show_bug.cgi?id=1484130#c37)
> >
> > '''
> > It is a kernel bug. The code snippet in comment 8 shows clearly that the
> > kernel
> > is doing the wrong thing, which cannot be fixed/worked around by QEMU.
> >
> > In man 2 fcntl:
> >
> > F_OFD_GETLK (struct flock *)
> >On input to this call, lock describes an open file
> >description lock
> > we would like to place on the file.  If the lock could  be  placed,
> > fcntl()  does  not
> >actually  place  it,  but  returns F_UNLCK in the l_type
> >field of lock
> > and leaves the other fields of the structure unchanged.  If one or more
> > incompatible
> >locks would prevent this lock being placed, then details
> >about one of
> > these locks are returned via lock, as described above for F_GETLK.
> >
> > which is not the case with the new CIFS behaviour.
> > ''
> >
> > You can read the full context at
> > https://bugzilla.redhat.com/show_bug.cgi?id=1484130
> >
> > Any suggestions?
> >
> > Thanks,
> > Laura
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> --
> Thanks,
> 
> Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] cifs: remove redundant duplicated assignment of pointer 'node'

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg <lsahl...@redhat.com>


- Original Message -
From: "Colin King" <colin.k...@canonical.com>
To: "Steve French" <sfre...@samba.org>, linux-c...@vger.kernel.org, 
samba-techni...@lists.samba.org
Cc: kernel-janit...@vger.kernel.org, linux-kernel@vger.kernel.org
Sent: Wednesday, 17 January, 2018 8:52:39 PM
Subject: [PATCH] cifs: remove redundant duplicated assignment of pointer 'node'

From: Colin Ian King <colin.k...@canonical.com>

Node is assigned twice to rb_first(root), first during declaration
time and second after a taking a spin lock, so we have a duplicated
assignment.  Remove the first assignment because it is redundant and
also not protected by the spin lock.

Cleans up clang warning:
fs/cifs/connect.c:4435:18: warning: Value stored to 'node' during
its initialization is never read

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
 fs/cifs/connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 0efd22e75ac7..63c5d85fe25e 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -4432,7 +4432,7 @@ cifs_prune_tlinks(struct work_struct *work)
struct cifs_sb_info *cifs_sb = container_of(work, struct cifs_sb_info,
prune_tlinks.work);
struct rb_root *root = _sb->tlink_tree;
-   struct rb_node *node = rb_first(root);
+   struct rb_node *node;
struct rb_node *tmp;
struct tcon_link *tlink;
 
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Overview of performance improvements of recent SMB3 compounding patches

[Ronnie Sahlberg]

We also have stat() that drops from 6 to 2 roundtrips.

For metadata I think the only remaining low hanging fruit is readdir().
Currently in cifs.ko scanning a directory takes a minimum of 4 roundtrips :

open
query and get data
query and get the error STATUS_NO_MORE_FILES
close


For small-ish directories (the common case I expect will be that almost all 
directories will fit in one reply of data)
we can cut this down to just a single roundtrip by using the compound :

open; query; query; close

If the second query fails with the error above we know we are done and we 
finished in a single
roundtrip.

The drawback is for the rare cases where the directory did not fit in the 
reply, then
we will have to throw all the data away and re-start from scratch using the old 
loop thus making all large directory scans
even slower than they are right now, well just one roundtrip slower but still.
(Technically, the protocol does support re-opening a directory and resuming at 
a specific FileIndex  but it is not implemented server-side.)

I think it will still be worth it if we can get the common case to drop from 4 
to 1 roundtrip.

It will involve touching cifs_readdir() which is hairy code but if someone 
would want to tackle this I think there is opportunity to make
a huge impact on directory listing performance.


regards
ronnie sahlberg




- Original Message -
> From: "Steve French" 
> To: "CIFS" , "samba-technical" 
> , "LKML"
> , "linux-fsdevel" 
> , "ronnie sahlberg"
> 
> Sent: Thursday, 23 August, 2018 5:28:01 AM
> Subject: Re: Overview of performance improvements of recent SMB3 compounding 
> patches
> 
> Continuing the experiments with Ronnie's patches show additional
> promising performance results from other common scenarios:
> 
> Very good news that the number of roundtrips (request/response pairs
> to the server) has dropped so substantially.  Reducing latency, and
> allowing the server to more efficiently process the requests leads to
> much better performance for these common operations:
> 
> - rename goes from 9 request/response pairs to 5 ("mv /mnt/file /mnt/file1")
> - hardlink goes from 8 to only 3 (!)  ("ln /mnt/file1 /mnt/file2")
> - symlink (with mfsymlinks enabled) goes from 11 to 9 ("ln -s
> /mnt/file1 /mnt/file3")
> - touch (existing file) 6 down to 4
> 
> In current kernel we benefit from compounding now on stafs ("stat -f
> /mnt"), and in the earlier note I described the improvements in
> create, unlink, mkdir and rmdir which were also awesome.
> 
> This is very exciting.
> 
> On Tue, Aug 21, 2018 at 1:24 PM Steve French  wrote:
> >
> > In experiments today with Ronnie's most recent compounding patches I
> > see the expected significant improvements in create/mkdir/unlink/rmdir
> > operations over SMB3 mounts (tests were to Samba but would be similar
> > to all modern servers).  See below:
> >
> > "touch /mnt/file"  goes from 6 request/response pairs to 4 with
> > Ronnie's compounding patches
> > "rm /mnt/file" from 5 to 2 request/response pairs
> > "mkdir /mnt/newdir" 6 pairs to 3 pairs
> > "rmdir /mnt/newdir" 6 pairs down to 2 pairs
> >
> > Good job Ronnie!
> > --
> > Thanks,
> >
> > Steve
> 
> 
> 
> --
> Thanks,
> 
> Steve
> 

Re: [PATCH 1/2] cifs: smbd: avoid reconnect lockup

[ronnie sahlberg]

Looks good to me (both patches)

Reviewed-by: Ronnie Sahlberg <lsahl...@redhat.com>

On Sat, Mar 31, 2018 at 8:16 AM, Long Li <lon...@linuxonhyperv.com> wrote:
> From: Long Li <lon...@microsoft.com>
>
> During transport reconnect, other processes may have registered memory
> and blocked on transport. This creates a deadlock situation because the
> transport resources can't be freed, and reconnect is blocked.
>
> Fix this by returning to upper layer on timeout. Before returning,
> transport status is set to reconnecting so other processes will release
> memory registration resources.
>
> Upper layer will retry the reconnect. This is not in fast I/O path so
> setting the timeout to 5 seconds.
>
> Signed-off-by: Long Li <lon...@microsoft.com>
> ---
>  fs/cifs/smbdirect.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index 5aa0b54..3f7883e 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -1498,8 +1498,8 @@ int smbd_reconnect(struct TCP_Server_Info *server)
> log_rdma_event(INFO, "reconnecting rdma session\n");
>
> if (!server->smbd_conn) {
> -   log_rdma_event(ERR, "rdma session already destroyed\n");
> -   return -EINVAL;
> +   log_rdma_event(INFO, "rdma session already destroyed\n");
> +   goto create_conn;
> }
>
> /*
> @@ -1512,15 +1512,19 @@ int smbd_reconnect(struct TCP_Server_Info *server)
> }
>
> /* wait until the transport is destroyed */
> -   wait_event(server->smbd_conn->wait_destroy,
> -   server->smbd_conn->transport_status == SMBD_DESTROYED);
> +   if (!wait_event_timeout(server->smbd_conn->wait_destroy,
> +   server->smbd_conn->transport_status == SMBD_DESTROYED, 5*HZ))
> +   return -EAGAIN;
>
> destroy_workqueue(server->smbd_conn->workqueue);
> kfree(server->smbd_conn);
>
> +create_conn:
> log_rdma_event(INFO, "creating rdma session\n");
> server->smbd_conn = smbd_get_connection(
> server, (struct sockaddr *) >dstaddr);
> +   log_rdma_event(INFO, "created rdma session info=%p\n",
> +   server->smbd_conn);
>
> return server->smbd_conn ? 0 : -ENOENT;
>  }
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: config files and how to have persistent Linux kernel Driver/File System configuration info saved

[ronnie sahlberg]

On Fri, Jun 29, 2018 at 8:58 AM, Theodore Y. Ts'o via samba-technical
 wrote:
> On Thu, Jun 28, 2018 at 05:37:15PM -0500, Steve French wrote:
>> Ronnie brought up an interesting point about the problems consistently
>> configuring file systems (or any Linux module for that matter) so that
>> reboot doesn't wipe away security or performance tuning changes.
>
> In general it's considered best practice to make the file system
> auto-tune itself as much as possible, because the sad fact is that
> 99.% of the customers aren't going to bother to add any tuning
> parameters.  So there hasn't been a push to try to create something
> more complex, because it's generally not needed.

True, but in these cases I think we are more looking at server or
mountpoint specific options than
actual fs tuning.

For example nfsmount.conf can be used to say "only use NFSv4 when
accessing server abc" etc.
For the case of CIFS I could imagine that an administrator might want
to set "disable smb1 protocol globally"
etc.

>
> Settings via /sys/fs/ext4 are generally for developers as they try to
> understand how things work, so they can improve the file systems
> defaults / auto-tuning algorithms.
>
> - Ted
>

Re: [PATCH v2] smb2: fix uninitialized variable bug in smb2_ioctl_query_info

[Ronnie Sahlberg]

Good catch, but I think it should be :

int resp_buftype = CIFS_NO_BUFFER;


- Original Message -
From: "Gustavo A. R. Silva" 
To: "Steve French" , "Ronnie Sahlberg" 
Cc: linux-c...@vger.kernel.org, samba-techni...@lists.samba.org, 
linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" 
Sent: Wednesday, 10 October, 2018 6:17:48 AM
Subject: [PATCH v2] smb2: fix uninitialized variable bug in 
smb2_ioctl_query_info

There is a potential execution path in which variable *resp_buftype*
is passed as an argument to function free_rsp_buf(), in which it is
used in a comparison without being properly initialized previously.

Fix this by initializing variable *resp_buftype* to -1 in order to
avoid unpredictable or unintended results.

Addresses-Coverity-ID: 1473971 ("Uninitialized scalar variable")
Fixes: c5d25bdb2967 ("cifs: add IOCTL for QUERY_INFO passthrough to userspace")
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Fix Coverity and Fixes tag.
 - Update commit log.

 fs/cifs/smb2ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index c6c6450d..927aadd 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1133,7 +1133,7 @@ smb2_ioctl_query_info(const unsigned int xid,
struct smb_rqst rqst;
struct kvec iov[1];
struct kvec rsp_iov;
-   int resp_buftype;
+   int resp_buftype = -1;
struct smb2_query_info_rsp *rsp = NULL;
void *buffer;
 
-- 
2.7.4

Re: Additional performance data on Pavel's smb3 multi credit patch series

[ronnie sahlberg]

Impressive!

On Sun, Jul 13, 2014 at 12:23 PM, Steve French  wrote:
> Performance of Pavel's multicredit i/o SMB3 patches continues to look
> good.  Additional informal performance results below comparing cifs
> mounts with smb3 mounts (vers=3.0) with and without Pavel's patch set.
> I plan to do additional testing with large rsize/wsize (default with
> Pavel's code is 1MB).
>
> 3.16-rc4 (Ubuntu) on client.  Server is Windows 8.1.  Both VMs on same
> host (host disk is fairly fast SSD).
>
> Copy to server performance increased about 20% percent
> dd if=/dev/zero of=/mnt/targetfile bs=80M count=25
> got similar results with or without conv=fdatasync
>
> 1st run copying to empty directory, 2nd run copying over targetfile,
> (pattern repeated multiple times) averaging results
>
> New code (with Pavel's patches)
> -
> CIFS 167MB/s
> SMB3 200MB/s
>
> Existing code (without his patches)
> 
> SMB3 166MB/s
> CIFS 164.5MB/s
>
> For large file reading SMB3 performance with Pavel's patches increased
> 76% over existing SMB3 code
> dd of=/dev/null if=/mnt/targetfile bs=80M count=25
> (mounting and unmounting between attempts to avoid caching effects on
> the client)
>
> New code (with Pavel's patches)
> -
> CIFS 114MB/s
> SMB3 216MB/s
>
> Existing code (without his patches)
> 
> SMB3 123MB/s
> CIFS 110MB/s
>
> --
> Thanks,
>
> Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2][SMB3] Add kernel trace support

[Ronnie Sahlberg]

Very nice.

Acked-by: Ronnie Sahlberg 

Possibly change the output from 
pid=6633 tid=0x0 sid=0x0 cmd=0 mid=0
to
cmd=0 mid=0 pid=6633 tid=0x0 sid=0x0

just to make it easier for human-searching. I think the cmd will be useful much 
more often than pid/tid/sid
and this would make it easier to look for as all cmd= entries will be aligned 
to the same column.



- Original Message -
From: "Steve French" 
To: "CIFS" , "LKML" , 
"samba-technical" , "linux-fsdevel" 

Sent: Friday, 18 May, 2018 12:36:36 PM
Subject: [PATCHv2][SMB3] Add kernel trace support

Patch updated with additional tracepoint locations and some formatting
improvements. There are some obvious additional tracepoints that could
be added, but this should be a reasonable group to start with.

>From edc02d6f9dc24963d510c7ef59067428d3b082d3 Mon Sep 17 00:00:00 2001
From: Steve French 
Date: Thu, 17 May 2018 21:16:55 -0500
Subject: [PATCH] smb3: Add ftrace tracepoints for improved SMB3 debugging

Although dmesg logs and wireshark network traces can be
helpful, being able to dynamically enable/disable tracepoints
(in this case via the kernel ftrace mechanism) can also be
helpful in more quickly debugging problems, and more
selectively tracing the events related to the bug report.

This patch adds 12 ftrace tracepoints to cifs.ko for SMB3 events
in some obvious locations.  Subsequent patches will add more
as needed.

Example use:
   trace-cmd record -e cifs
   
   trace-cmd show

Various trace events can be filtered. See:
   trace-cmd list | grep cifs
for the current list of cifs tracepoints.

Sample output (from mount and writing to a file):

root@smf:/sys/kernel/debug/tracing/events/cifs# trace-cmd show

  mount.cifs-6633  [006]   7246.936461: smb3_cmd_done:
pid=6633 tid=0x0 sid=0x0 cmd=0 mid=0
  mount.cifs-6633  [006]   7246.936701: smb3_cmd_err:
pid=6633 tid=0x0 sid=0x3d9cf8e5 cmd=1 mid=1 status=0xc016 rc=-5
  mount.cifs-6633  [006]   7246.943055: smb3_cmd_done:
pid=6633 tid=0x0 sid=0x3d9cf8e5 cmd=1 mid=2
  mount.cifs-6633  [006]   7246.943298: smb3_cmd_done:
pid=6633 tid=0xf9447636 sid=0x3d9cf8e5 cmd=3 mid=3
  mount.cifs-6633  [006]   7246.943446: smb3_cmd_done:
pid=6633 tid=0xf9447636 sid=0x3d9cf8e5 cmd=11 mid=4
  mount.cifs-6633  [006]   7246.943659: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=3 mid=5
  mount.cifs-6633  [006]   7246.943766: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=11 mid=6
  mount.cifs-6633  [006]   7246.943937: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=5 mid=7
  mount.cifs-6633  [006]   7246.944020: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=8
  mount.cifs-6633  [006]   7246.944091: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=9
  mount.cifs-6633  [006]   7246.944163: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=10
  mount.cifs-6633  [006]   7246.944218: smb3_cmd_err:
pid=6633 tid=0xf9447636 sid=0x3d9cf8e5 cmd=11 mid=11 status=0xc225
rc=-2
  mount.cifs-6633  [006]   7246.944219: smb3_fsctl_err: xid=0
fid=0x tid=0xf9447636 sid=0x3d9cf8e5 class=0
type=393620 rc=-2
  mount.cifs-6633  [007]   7246.944353: smb3_cmd_done:
pid=6633 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=12
bash-2071  [000]   7256.903844: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=5 mid=13
bash-2071  [000]   7256.904172: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=16 mid=14
bash-2071  [000]   7256.904471: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=17 mid=15
bash-2071  [000]   7256.904950: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=5 mid=16
bash-2071  [000]   7256.905305: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=17 mid=17
bash-2071  [000]   7256.905688: smb3_cmd_done:
pid=2071 tid=0xe1b781a sid=0x3d9cf8e5 cmd=6 mid=18
bash-2071  [000]   7256.905809: smb3_write_done: xid=0
fid=0xd628f511 tid=0xe1b781a sid=0x3d9cf8e5 offset=0x0 len=0x1b

Signed-off-by: Steve French 
---
 fs/cifs/Makefile   |   7 +-
 fs/cifs/smb2maperror.c |  10 +-
 fs/cifs/smb2pdu.c  |  56 +++-
 fs/cifs/trace.c|  18 +++
 fs/cifs/trace.h| 298 +
 5 files changed, 379 insertions(+), 10 deletions(-)
 create mode 100644 fs/cifs/trace.c
 create mode 100644 fs/cifs/trace.h

diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile
index 7e4a1e2f0696..85817991ee68 100644
--- a/fs/cifs/Makefile
+++ b/fs/cifs/Makefile
@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-# Makefile for Linux CIFS VFS client
+# Makefile for Linux CIFS/SMB2/SMB3 VFS client
 #
+ccflags-y += -I$(src)# needed for trace events
 obj-$(CONFIG_CIFS) += cifs.o

-cifs-y := cifsfs.o 

Re: [PATCHv2][SMB3] Add kernel trace support

[ronnie sahlberg]

Fair enough.

Onto a second point then, for future patches.

There are a lot of places where we do not (yet) pass a tcon as argument.
Can we make a policy decision to mandate that every tracepoint MUST
log tid, sid?

And thus, if you need a new tracepoint in a function where tcon is not
available you then first
have to do the plumbing to pass the tcon all the way down to the tracepoint?


Ideally we should pass tcon into any and every leaf function for
tracepoints as well as debug logging.


On Fri, May 18, 2018 at 4:19 PM, Steve French  wrote:
> On Thu, May 17, 2018 at 10:28 PM, Ronnie Sahlberg  wrote:
>> Very nice.
>>
>> Acked-by: Ronnie Sahlberg 
>>
>> Possibly change the output from
>> pid=6633 tid=0x0 sid=0x0 cmd=0 mid=0
>> to
>> cmd=0 mid=0 pid=6633 tid=0x0 sid=0x0
>>
>> just to make it easier for human-searching. I think the cmd will be useful 
>> much more often than pid/tid/sid
>> and this would make it easier to look for as all cmd= entries will be 
>> aligned to the same column.
>
> My instinct is to preserve the consistency by beginning with the the
> fields that will be in 90% of the commands: tree id and session id
> (tid and sid), which would cause pid to move after sid or after cmd,
> but I would prefer to wait on reordering fields and fixing alignment
> till we add another set of tracepoints (e.g. in FreeXid, and in
> SMB2_open and in the caller of negprot/sessionsetup) - we should then
> have a better idea what formatting would make it slightly more
> consistent and readable.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v7 05/22] CIFS: SMBD: Establish SMB Direct connection

[ronnie sahlberg]

On Tue, Nov 7, 2017 at 6:54 PM, Long Li  wrote:
> From: Long Li 
>
> Add code to implement the core functions to establish a SMB Direct connection.
>
> 1. Establish an RDMA connection to SMB server.
> 2. Negotiate and setup SMB Direct protocol.
> 3. Implement idle connection timer and credit management.
>
> SMB Direct is enabled by setting CONFIG_CIFS_SMB_DIRECT.
>
> Add to Makefile to enable building SMB Direct.
>
> Signed-off-by: Long Li 
> ---
>  fs/cifs/Makefile|2 +
>  fs/cifs/smbdirect.c | 1576 
> +++
>  fs/cifs/smbdirect.h |  280 +
>  3 files changed, 1858 insertions(+)
>
> diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile
> index 5e853a3..ad00873 100644
> --- a/fs/cifs/Makefile
> +++ b/fs/cifs/Makefile
> @@ -18,3 +18,5 @@ cifs-$(CONFIG_CIFS_UPCALL) += cifs_spnego.o
>  cifs-$(CONFIG_CIFS_DFS_UPCALL) += dns_resolve.o cifs_dfs_ref.o
>
>  cifs-$(CONFIG_CIFS_FSCACHE) += fscache.o cache.o
> +
> +cifs-$(CONFIG_CIFS_SMB_DIRECT) += smbdirect.o
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index d3c16f8..021d527 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -13,7 +13,34 @@
>   *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>   *   the GNU General Public License for more details.
>   */
> +#include 
>  #include "smbdirect.h"
> +#include "cifs_debug.h"
> +
> +static struct smbd_response *get_empty_queue_buffer(
> +   struct smbd_connection *info);
> +static struct smbd_response *get_receive_buffer(
> +   struct smbd_connection *info);
> +static void put_receive_buffer(
> +   struct smbd_connection *info,
> +   struct smbd_response *response,
> +   bool lock);
> +static int allocate_receive_buffers(struct smbd_connection *info, int 
> num_buf);
> +static void destroy_receive_buffers(struct smbd_connection *info);
> +
> +static void put_empty_packet(
> +   struct smbd_connection *info, struct smbd_response *response);
> +static void enqueue_reassembly(
> +   struct smbd_connection *info,
> +   struct smbd_response *response, int data_length);
> +static struct smbd_response *_get_first_reassembly(
> +   struct smbd_connection *info);
> +
> +static int smbd_post_recv(
> +   struct smbd_connection *info,
> +   struct smbd_response *response);
> +
> +static int smbd_post_send_empty(struct smbd_connection *info);
>
>  /* SMBD version number */
>  #define SMBD_V10x0100
> @@ -75,3 +102,1552 @@ int smbd_max_frmr_depth = 2048;
>
>  /* If payload is less than this byte, use RDMA send/recv not read/write */
>  int rdma_readwrite_threshold = 4096;
> +
> +/* Transport logging functions
> + * Logging are defined as classes. They can be OR'ed to define the actual
> + * logging level via module parameter smbd_logging_class
> + * e.g. cifs.smbd_logging_class=0x500 will log all log_rdma_recv() and
> + * log_rdma_event()

Should be 0xa0 ?

> + */
> +#define LOG_OUTGOING   0x1
> +#define LOG_INCOMING   0x2
> +#define LOG_READ   0x4
> +#define LOG_WRITE  0x8
> +#define LOG_RDMA_SEND  0x10
> +#define LOG_RDMA_RECV  0x20
> +#define LOG_KEEP_ALIVE 0x40
> +#define LOG_RDMA_EVENT 0x80
> +#define LOG_RDMA_MR0x100
> +static unsigned int smbd_logging_class = 0;
> +module_param(smbd_logging_class, uint, 0644);
> +MODULE_PARM_DESC(smbd_logging_class,
> +   "Logging class for SMBD transport 0x0 to 0x100");
> +
> +#define ERR0x0
> +#define INFO   0x1
> +static unsigned int smbd_logging_level = ERR;
> +module_param(smbd_logging_level, uint, 0644);
> +MODULE_PARM_DESC(smbd_logging_level,
> +   "Logging level for SMBD transport, 0 (default): error, 1: info");
> +
> +#define log_rdma(level, class, fmt, args...)   \
> +do {   \
> +   if (level <= smbd_logging_level || class & smbd_logging_class)  \
> +   cifs_dbg(VFS, "%s:%d " fmt, __func__, __LINE__, ##args);\
> +} while (0)
> +
> +#define log_outgoing(level, fmt, args...) \
> +   log_rdma(level, LOG_OUTGOING, fmt, ##args)
> +#define log_incoming(level, fmt, args...) \
> +   log_rdma(level, LOG_INCOMING, fmt, ##args)
> +#define log_read(level, fmt, args...)  log_rdma(level, LOG_READ, fmt, ##args)
> +#define log_write(level, fmt, args...) log_rdma(level, LOG_WRITE, fmt, 
> ##args)
> +#define log_rdma_send(level, fmt, args...) \
> +   log_rdma(level, LOG_RDMA_SEND, fmt, ##args)
> +#define log_rdma_recv(level, fmt, args...) \
> +   log_rdma(level, LOG_RDMA_RECV, fmt, ##args)
> +#define log_keep_alive(level, fmt, args...) \
> +   log_rdma(level, LOG_KEEP_ALIVE, fmt, ##args)
> +#define log_rdma_event(level, fmt, 

Re: [PATCH 1/2] cifs: smbd: avoid reconnect lockup

[ronnie sahlberg]

Looks good to me (both patches)

Reviewed-by: Ronnie Sahlberg 

On Sat, Mar 31, 2018 at 8:16 AM, Long Li  wrote:
> From: Long Li 
>
> During transport reconnect, other processes may have registered memory
> and blocked on transport. This creates a deadlock situation because the
> transport resources can't be freed, and reconnect is blocked.
>
> Fix this by returning to upper layer on timeout. Before returning,
> transport status is set to reconnecting so other processes will release
> memory registration resources.
>
> Upper layer will retry the reconnect. This is not in fast I/O path so
> setting the timeout to 5 seconds.
>
> Signed-off-by: Long Li 
> ---
>  fs/cifs/smbdirect.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index 5aa0b54..3f7883e 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -1498,8 +1498,8 @@ int smbd_reconnect(struct TCP_Server_Info *server)
> log_rdma_event(INFO, "reconnecting rdma session\n");
>
> if (!server->smbd_conn) {
> -   log_rdma_event(ERR, "rdma session already destroyed\n");
> -   return -EINVAL;
> +   log_rdma_event(INFO, "rdma session already destroyed\n");
> +   goto create_conn;
> }
>
> /*
> @@ -1512,15 +1512,19 @@ int smbd_reconnect(struct TCP_Server_Info *server)
> }
>
> /* wait until the transport is destroyed */
> -   wait_event(server->smbd_conn->wait_destroy,
> -   server->smbd_conn->transport_status == SMBD_DESTROYED);
> +   if (!wait_event_timeout(server->smbd_conn->wait_destroy,
> +   server->smbd_conn->transport_status == SMBD_DESTROYED, 5*HZ))
> +   return -EAGAIN;
>
> destroy_workqueue(server->smbd_conn->workqueue);
> kfree(server->smbd_conn);
>
> +create_conn:
> log_rdma_event(INFO, "creating rdma session\n");
> server->smbd_conn = smbd_get_connection(
> server, (struct sockaddr *) >dstaddr);
> +   log_rdma_event(INFO, "created rdma session info=%p\n",
> +   server->smbd_conn);
>
> return server->smbd_conn ? 0 : -ENOENT;
>  }
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH AUTOSEL 5.2 039/249] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig

[ronnie sahlberg]

On Tue, Jul 16, 2019 at 1:15 AM Sasha Levin  wrote:
>
> From: "Eric W. Biederman" 
>
> [ Upstream commit 72abe3bcf0911d69b46c1e8bdb5612675e0ac42c ]
>
> The locking in force_sig_info is not prepared to deal with a task that
> exits or execs (as sighand may change).  The is not a locking problem
> in force_sig as force_sig is only built to handle synchronous
> exceptions.
>
> Further the function force_sig_info changes the signal state if the
> signal is ignored, or blocked or if SIGNAL_UNKILLABLE will prevent the
> delivery of the signal.  The signal SIGKILL can not be ignored and can
> not be blocked and SIGNAL_UNKILLABLE won't prevent it from being
> delivered.
>
> So using force_sig rather than send_sig for SIGKILL is confusing
> and pointless.
>
> Because it won't impact the sending of the signal and and because
> using force_sig is wrong, replace force_sig with send_sig.

I think this patch broke the cifs module.
The issue is that the use count is now not updated properly and thus
it is no longer possible to
rmmod the module.


>
> Cc: Namjae Jeon 
> Cc: Jeff Layton 
> Cc: Steve French 
> Fixes: a5c3e1c725af ("Revert "cifs: No need to send SIGKILL to demux_thread 
> during umount"")
> Fixes: e7ddee9037e7 ("cifs: disable sharing session and tcon and add new TCP 
> sharing code")
> Signed-off-by: "Eric W. Biederman" 
> Signed-off-by: Sasha Levin 
> ---
>  fs/cifs/connect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8dd6637a3cbb..714a359c7c8d 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2631,7 +2631,7 @@ cifs_put_tcp_session(struct TCP_Server_Info *server, 
> int from_reconnect)
>
> task = xchg(>tsk, NULL);
> if (task)
> -   force_sig(SIGKILL, task);
> +   send_sig(SIGKILL, task, 1);
>  }
>
>  static struct TCP_Server_Info *
> --
> 2.20.1
>

Re: [PATCH] cifs: remove redundant duplicated assignment of pointer 'node'

[Ronnie Sahlberg]

Reviewed-by: Ronnie Sahlberg 


- Original Message -
From: "Colin King" 
To: "Steve French" , linux-c...@vger.kernel.org, 
samba-techni...@lists.samba.org
Cc: kernel-janit...@vger.kernel.org, linux-kernel@vger.kernel.org
Sent: Wednesday, 17 January, 2018 8:52:39 PM
Subject: [PATCH] cifs: remove redundant duplicated assignment of pointer 'node'

From: Colin Ian King 

Node is assigned twice to rb_first(root), first during declaration
time and second after a taking a spin lock, so we have a duplicated
assignment.  Remove the first assignment because it is redundant and
also not protected by the spin lock.

Cleans up clang warning:
fs/cifs/connect.c:4435:18: warning: Value stored to 'node' during
its initialization is never read

Signed-off-by: Colin Ian King 
---
 fs/cifs/connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 0efd22e75ac7..63c5d85fe25e 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -4432,7 +4432,7 @@ cifs_prune_tlinks(struct work_struct *work)
struct cifs_sb_info *cifs_sb = container_of(work, struct cifs_sb_info,
prune_tlinks.work);
struct rb_root *root = _sb->tlink_tree;
-   struct rb_node *node = rb_first(root);
+   struct rb_node *node;
struct rb_node *tmp;
struct tcon_link *tlink;
 
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][cifs-next] cifs: ensure variable rc is initialized at the after_open label

[ronnie sahlberg]

On Thu, Aug 29, 2019 at 2:00 PM Steve French  wrote:
>
> Merged into cifs-2.6.git for-next
>
> Ronnie,
> You ok with merging this as a distinct patch?

Sure thing.
Thanks for the fix Colin.


>
> On Wed, Aug 28, 2019 at 7:02 PM Colin King  wrote:
> >
> > From: Colin Ian King 
> >
> > A previous fix added a jump to after_open which now leaves variable
> > rc in a uninitialized state. A couple of the cases in the following
> > switch statement do not set variable rc, hence the error check on rc
> > at the end of the switch statement is reading a garbage value in rc
> > for those specific cases. Fix this by initializing rc to zero before
> > the switch statement.
> >
> > Fixes: 955a9c5b39379 ("cifs: create a helper to find a writeable handle by 
> > path name")
> > Addresses-Coverity: ("Uninitialized scalar variable")
> > Signed-off-by: Colin Ian King 
> > ---
> >  fs/cifs/smb2inode.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
> > index 70342bcd89b4..939fc7b2234c 100644
> > --- a/fs/cifs/smb2inode.c
> > +++ b/fs/cifs/smb2inode.c
> > @@ -116,6 +116,7 @@ smb2_compound_op(const unsigned int xid, struct 
> > cifs_tcon *tcon,
> > smb2_set_next_command(tcon, [num_rqst]);
> >   after_open:
> > num_rqst++;
> > +   rc = 0;
> >
> > /* Operation */
> > switch (command) {
> > --
> > 2.20.1
> >
>
>
> --
> Thanks,
>
> Steve

Re: F_OFD_GETLK implemented wrong with CIFS protocol version 2.0+

[Ronnie Sahlberg]

The problem is in fs/cifs/file.c:cifs_find_fid_lock_conflict since it is not 
aware of OFD locks
and thus think there is a conflict.

I have an initial patch that fixes the problem for the reproducer but need more 
time to understand if/what
else might need fixin.





- Original Message -
> From: "Steve French" 
> To: labb...@redhat.com
> Cc: "CIFS" , "samba-technical" 
> , "LKML"
> , "Adam Williamson" 
> Sent: Tuesday, 26 June, 2018 1:54:40 PM
> Subject: Re: F_OFD_GETLK implemented wrong with CIFS protocol version 2.0+
> 
> We are taking a look at this - Ronnie had some ideas.  Probably simply
> not implemented - hopefully not too hard to fix.
> On Mon, Jun 25, 2018 at 6:58 PM Laura Abbott  wrote:
> >
> > Hi,
> >
> > A while back, someone reported a failure on Fedora when trying to boot
> > a QEMU image off of a CIFS share. The issue was reduced down to a
> > test case (https://bugzilla.redhat.com/show_bug.cgi?id=1484130#c8)
> >
> > # cat test-ofd-lock.c
> > #define _GNU_SOURCE
> > #include 
> > #include 
> > #include 
> > #include 
> >
> > int main(int argc, char **argv)
> > {
> >  int ret;
> >  int fd;
> >  struct flock fl = {
> >  .l_whence = SEEK_SET,
> >  .l_start  = 0,
> >  .l_len= 0,
> >  .l_type   = F_RDLCK,
> >  };
> >  if (argc < 2) {
> >  fprintf(stderr, "Usage: %s \n", argv[0]);
> >  return 1;
> >  }
> >  fd = open(argv[1], O_RDWR);
> >  if (fd < 0) {
> >  perror("open");
> >  return errno;
> >  }
> >  ret = fcntl(fd, F_OFD_SETLK, );
> >  if (ret) {
> >  perror("setlk");
> >  return errno;
> >  }
> >  fl.l_type = F_WRLCK;
> >  ret = fcntl(fd, F_OFD_GETLK, );
> >  if (ret) {
> >  perror("getlk");
> >  return errno;
> >  }
> >  if (fl.l_type != F_UNLCK) {
> >  fprintf(stderr, "get lock test failed\n");
> >  return 1;
> >  }
> >  return 0;
> > }
> > [root@localhost ~]# make test-ofd-lock
> > cc test-ofd-lock.c   -o test-ofd-lock
> > [root@localhost ~]# touch /tmp/test && ./test-ofd-lock /tmp/test
> > [root@localhost ~]# echo $?
> > 0
> > [root@localhost ~]# touch /mnt/test && ./test-ofd-lock /mnt/test
> > get lock test failed
> > [root@localhost ~]# mount | grep /mnt
> > //192.168.31.1/tddownload on /mnt type cifs (rw,relatime,vers=3.0,
> > cache=strict,username=admin,domain=,uid=0,
> > noforceuid,gid=0,noforcegid,addr=192.168.31.1,file_mode=0755,
> > dir_mode=0755,nounix,serverino,mapposix,rsize=1048576,
> > wsize=1048576,echo_interval=60,actimeo=1,user=admin)
> >
> >
> > As explained by one of the QEMU developers
> > (https://bugzilla.redhat.com/show_bug.cgi?id=1484130#c37)
> >
> > '''
> > It is a kernel bug. The code snippet in comment 8 shows clearly that the
> > kernel
> > is doing the wrong thing, which cannot be fixed/worked around by QEMU.
> >
> > In man 2 fcntl:
> >
> > F_OFD_GETLK (struct flock *)
> >On input to this call, lock describes an open file
> >description lock
> > we would like to place on the file.  If the lock could  be  placed,
> > fcntl()  does  not
> >actually  place  it,  but  returns F_UNLCK in the l_type
> >field of lock
> > and leaves the other fields of the structure unchanged.  If one or more
> > incompatible
> >locks would prevent this lock being placed, then details
> >about one of
> > these locks are returned via lock, as described above for F_GETLK.
> >
> > which is not the case with the new CIFS behaviour.
> > ''
> >
> > You can read the full context at
> > https://bugzilla.redhat.com/show_bug.cgi?id=1484130
> >
> > Any suggestions?
> >
> > Thanks,
> > Laura
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> --
> Thanks,
> 
> Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: config files and how to have persistent Linux kernel Driver/File System configuration info saved

[ronnie sahlberg]

On Fri, Jun 29, 2018 at 8:58 AM, Theodore Y. Ts'o via samba-technical
 wrote:
> On Thu, Jun 28, 2018 at 05:37:15PM -0500, Steve French wrote:
>> Ronnie brought up an interesting point about the problems consistently
>> configuring file systems (or any Linux module for that matter) so that
>> reboot doesn't wipe away security or performance tuning changes.
>
> In general it's considered best practice to make the file system
> auto-tune itself as much as possible, because the sad fact is that
> 99.% of the customers aren't going to bother to add any tuning
> parameters.  So there hasn't been a push to try to create something
> more complex, because it's generally not needed.

True, but in these cases I think we are more looking at server or
mountpoint specific options than
actual fs tuning.

For example nfsmount.conf can be used to say "only use NFSv4 when
accessing server abc" etc.
For the case of CIFS I could imagine that an administrator might want
to set "disable smb1 protocol globally"
etc.

>
> Settings via /sys/fs/ext4 are generally for developers as they try to
> understand how things work, so they can improve the file systems
> defaults / auto-tuning algorithms.
>
> - Ted
>

Re: Overview of performance improvements of recent SMB3 compounding patches

[Ronnie Sahlberg]

We also have stat() that drops from 6 to 2 roundtrips.

For metadata I think the only remaining low hanging fruit is readdir().
Currently in cifs.ko scanning a directory takes a minimum of 4 roundtrips :

open
query and get data
query and get the error STATUS_NO_MORE_FILES
close


For small-ish directories (the common case I expect will be that almost all 
directories will fit in one reply of data)
we can cut this down to just a single roundtrip by using the compound :

open; query; query; close

If the second query fails with the error above we know we are done and we 
finished in a single
roundtrip.

The drawback is for the rare cases where the directory did not fit in the 
reply, then
we will have to throw all the data away and re-start from scratch using the old 
loop thus making all large directory scans
even slower than they are right now, well just one roundtrip slower but still.
(Technically, the protocol does support re-opening a directory and resuming at 
a specific FileIndex  but it is not implemented server-side.)

I think it will still be worth it if we can get the common case to drop from 4 
to 1 roundtrip.

It will involve touching cifs_readdir() which is hairy code but if someone 
would want to tackle this I think there is opportunity to make
a huge impact on directory listing performance.


regards
ronnie sahlberg




- Original Message -
> From: "Steve French" 
> To: "CIFS" , "samba-technical" 
> , "LKML"
> , "linux-fsdevel" 
> , "ronnie sahlberg"
> 
> Sent: Thursday, 23 August, 2018 5:28:01 AM
> Subject: Re: Overview of performance improvements of recent SMB3 compounding 
> patches
> 
> Continuing the experiments with Ronnie's patches show additional
> promising performance results from other common scenarios:
> 
> Very good news that the number of roundtrips (request/response pairs
> to the server) has dropped so substantially.  Reducing latency, and
> allowing the server to more efficiently process the requests leads to
> much better performance for these common operations:
> 
> - rename goes from 9 request/response pairs to 5 ("mv /mnt/file /mnt/file1")
> - hardlink goes from 8 to only 3 (!)  ("ln /mnt/file1 /mnt/file2")
> - symlink (with mfsymlinks enabled) goes from 11 to 9 ("ln -s
> /mnt/file1 /mnt/file3")
> - touch (existing file) 6 down to 4
> 
> In current kernel we benefit from compounding now on stafs ("stat -f
> /mnt"), and in the earlier note I described the improvements in
> create, unlink, mkdir and rmdir which were also awesome.
> 
> This is very exciting.
> 
> On Tue, Aug 21, 2018 at 1:24 PM Steve French  wrote:
> >
> > In experiments today with Ronnie's most recent compounding patches I
> > see the expected significant improvements in create/mkdir/unlink/rmdir
> > operations over SMB3 mounts (tests were to Samba but would be similar
> > to all modern servers).  See below:
> >
> > "touch /mnt/file"  goes from 6 request/response pairs to 4 with
> > Ronnie's compounding patches
> > "rm /mnt/file" from 5 to 2 request/response pairs
> > "mkdir /mnt/newdir" 6 pairs to 3 pairs
> > "rmdir /mnt/newdir" 6 pairs down to 2 pairs
> >
> > Good job Ronnie!
> > --
> > Thanks,
> >
> > Steve
> 
> 
> 
> --
> Thanks,
> 
> Steve
> 

Re: [PATCH 2/2] cifs: Allocate memory for all iovs in smb2_ioctl

[ronnie sahlberg]

On Thu, May 16, 2019 at 7:10 AM  wrote:
>
> From: Long Li 
>
> An IOCTL uses up to 2 iovs. The 1st iov is the command itself, the 2nd iov is
> optional data for that command. The 1st iov is always allocated on the heap
> but the 2nd iov may point to a variable on the stack. This will trigger an
> error when passing the 2nd iov for RDMA I/O.
>
> Fix this by allocating a buffer for the 2nd iov.
>
> Signed-off-by: Long Li 
> ---
>  fs/cifs/smb2pdu.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 29f011d8d8e2..710ceb875161 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2538,11 +2538,25 @@ SMB2_ioctl_init(struct cifs_tcon *tcon, struct 
> smb_rqst *rqst,
> struct kvec *iov = rqst->rq_iov;
> unsigned int total_len;
> int rc;
> +   char *in_data_buf;
>
> rc = smb2_plain_req_init(SMB2_IOCTL, tcon, (void **) , 
> _len);
> if (rc)
> return rc;
>
> +   if (indatalen) {
> +   /*
> +* indatalen is usually small at a couple of bytes max, so
> +* just allocate through generic pool
> +*/
> +   in_data_buf = kmalloc(indatalen, GFP_NOFS);
> +   if (!in_data_buf) {
> +   cifs_small_buf_release(req);
> +   return -ENOMEM;
> +   }
> +   memcpy(in_data_buf, in_data, indatalen);
> +   }
> +
> req->CtlCode = cpu_to_le32(opcode);
> req->PersistentFileId = persistent_fid;
> req->VolatileFileId = volatile_fid;
> @@ -2563,7 +2577,7 @@ SMB2_ioctl_init(struct cifs_tcon *tcon, struct smb_rqst 
> *rqst,
>cpu_to_le32(offsetof(struct smb2_ioctl_req, Buffer));
> rqst->rq_nvec = 2;
> iov[0].iov_len = total_len - 1;
> -   iov[1].iov_base = in_data;
> +   iov[1].iov_base = in_data_buf;
> iov[1].iov_len = indatalen;
> } else {
> rqst->rq_nvec = 1;
> @@ -2605,8 +2619,11 @@ SMB2_ioctl_init(struct cifs_tcon *tcon, struct 
> smb_rqst *rqst,
>  void
>  SMB2_ioctl_free(struct smb_rqst *rqst)
>  {
> -   if (rqst && rqst->rq_iov)
> +   if (rqst && rqst->rq_iov) {
> cifs_small_buf_release(rqst->rq_iov[0].iov_base); /* request 
> */
> +   if (rqst->rq_iov[1].iov_len)
> +   kfree(rqst->rq_iov[1].iov_base);

You don't need the conditional. kfree(NULL) is safe,.

> +   }
>  }
>
>
> --
> 2.17.1
>

Reviewed-by: Ronnie sahlberg 

Re: [PATCH] [CIFS] Remove unneeded statements pointed out by Coverity

[ronnie sahlberg]

reviewed by me

On Thu, Mar 14, 2019 at 7:53 AM Steve French  wrote:
>
> cifs: remove unused value pointed out by Coverity
>
> Detected by CoverityScan CID#1438719 ("Unused Value")
>
> buf is reset again before being used so these two lines of code
> are useless.
>
> Signed-off-by: Steve French 
> ---
>  fs/cifs/connect.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index b95db2b593cb..a8e9738db691 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1191,10 +1191,6 @@ cifs_demultiplex_thread(void *p)
>  continue;
>  }
>
> -if (server->large_buf)
> -buf = server->bigbuf;
> -
> -
>  server->lstrp = jiffies;
>
>  for (i = 0; i < num_mids; i++) {
>
> --
> Thanks,
>
> Steve

Re: linux-next: manual merge of the cifs tree with Linus' tree

[Ronnie Sahlberg]

Steve,

Looks good. It builds and seems to work in my testing.
Thanks.

- Original Message -
From: "Steve French" 
To: "Stephen Rothwell" 
Cc: "CIFS" , "Linux-Next Mailing List" 
, "Linux Kernel Mailing List" 
, "Kees Cook" , "Ronnie 
Sahlberg" , "Aurelien Aptel" 
Sent: Thursday, 14 June, 2018 10:40:43 AM
Subject: Re: linux-next: manual merge of the cifs tree with Linus' tree

Ronnie,

I fixed up that patch and repushed to cifs-2.6.git for-next.  Seems
trivial. Let me know if any issues.

On Wed, Jun 13, 2018 at 6:01 PM, Stephen Rothwell  wrote:
> Hi all,
>
> Today's linux-next merge of the cifs tree got a conflict in:
>
>   fs/cifs/transport.c
>
> between commit:
>
>   6da2ec56059c ("treewide: kmalloc() -> kmalloc_array()")
>
> from Linus' tree and commit:
>
>   1560d69e21c6 ("cifs: push rfc1002 generation down the stack")
>
> from the cifs tree.
>
> I fixed it up (the latter removed the code modified by the former) and
> can carry the fix as necessary. This is now fixed as far as linux-next
> is concerned, but any non trivial conflicts should be mentioned to your
> upstream maintainer when your tree is submitted for merging.  You may
> also want to consider cooperating with the maintainer of the conflicting
> tree to minimise any particularly complex conflicts.
>
> --
> Cheers,
> Stephen Rothwell



-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] smb2: fix uninitialized variable bug in smb2_ioctl_query_info

[Ronnie Sahlberg]

Good catch, but I think it should be :

int resp_buftype = CIFS_NO_BUFFER;


- Original Message -
From: "Gustavo A. R. Silva" 
To: "Steve French" , "Ronnie Sahlberg" 
Cc: linux-c...@vger.kernel.org, samba-techni...@lists.samba.org, 
linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" 
Sent: Wednesday, 10 October, 2018 6:17:48 AM
Subject: [PATCH v2] smb2: fix uninitialized variable bug in 
smb2_ioctl_query_info

There is a potential execution path in which variable *resp_buftype*
is passed as an argument to function free_rsp_buf(), in which it is
used in a comparison without being properly initialized previously.

Fix this by initializing variable *resp_buftype* to -1 in order to
avoid unpredictable or unintended results.

Addresses-Coverity-ID: 1473971 ("Uninitialized scalar variable")
Fixes: c5d25bdb2967 ("cifs: add IOCTL for QUERY_INFO passthrough to userspace")
Signed-off-by: Gustavo A. R. Silva 
---
Changes in v2:
 - Fix Coverity and Fixes tag.
 - Update commit log.

 fs/cifs/smb2ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index c6c6450d..927aadd 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1133,7 +1133,7 @@ smb2_ioctl_query_info(const unsigned int xid,
struct smb_rqst rqst;
struct kvec iov[1];
struct kvec rsp_iov;
-   int resp_buftype;
+   int resp_buftype = -1;
struct smb2_query_info_rsp *rsp = NULL;
void *buffer;
 
-- 
2.7.4

Re: [Linux-cifsd-devel] [PATCH] cifsd: use kfree to free memory allocated by kzalloc

[ronnie sahlberg]

On Fri, Apr 2, 2021 at 7:04 AM Tom Talpey  wrote:
>
> On 4/1/2021 9:36 AM, Namjae Jeon wrote:
> > 2021-04-01 22:14 GMT+09:00, Ralph Boehme :
> >> Am 4/1/21 um 2:59 PM schrieb Namjae Jeon:
> >>> 2021-04-01 21:50 GMT+09:00, Ralph Boehme :
> >>>> fwiw, while at it what about renaming everything that still references
> >>>> "cifs" to "smb" ? This is not the 90's... :)
> >>> It is also used with the name "ksmbd". So function and variable prefix
> >>> are used with ksmbd.
> >>
> >> well, I was thinking of this:
> >>
> >>   > +++ b/fs/cifsd/...
> >>
> >> We should really stop using the name cifs for modern implementation of
> >> SMB{23} and the code should not be added as fs/cifsd/ to the kernel.
> > As I know, currently "cifs" is being used for the subdirectory name
> > for historical reasons and to avoid confusions, even though the CIFS
> > (SMB1) dialect is no longer recommended.
>
> I'm with Ralph. CIFS is history that we need to relegate to the past.

Tom, and Ralph.
Some background on the cifsd directory name:

We discussed in length but we decided with cifsd to align with the
current directory name cifs for the client.
Just to align with current praxis defined by other filesystems, i.e.
nfs. which has nfs for client, nfsd for server
and nfs_common for shared cod and definitions.

Once cifsd lands in the kernel I expect we will start building
cifs_common for this purpose.

An alternative would have been to rename the current fs/cifs tree to
fs/ksmb but renaming an entire directory tree
felt it might get pushback.
In the end we thought that the module name, that is user visible and
there it is important we call it smb3 something
but the source tree is not end-user visible so it was less important
what the name was.

(the alternative ending up with   fs/cifs  fs/ksmbd and fs/cifs_common
would have been terrible)

regards
ronnie sahlberg

>
> I also agree that wrappers around core memory allocators are to
> be avoided.
>
> Tom.

Re: [PATCH] CIFS: Prevent error log on spurious oplock break

[ronnie sahlberg]

On Sun, Mar 7, 2021 at 8:52 PM Shyam Prasad N via samba-technical
 wrote:
>
> Hi Vincent,
>
> The reason for rejecting the request maybe a number of things like:
> corrupted request, stale request (for some old session), or for a
> wrong handle.
> I don't think we should treat any of these cases as a success.

I agree with Shyam here.
We shouldn't change the return value to pretend success just to
suppress a warning.

However, if it is common to trigger with false positives we might want
to something to prevent it from
spamming the logs.
These messages could be useful if we encounter bugs in our leasing
code, or bugs in server
lease code, so we should't throw them away completely. But if false
positives are common ...

Some thoughts I and Stever brainstormed about could be to change the code in the
demiltiplex thread where we currently dump the packets that were "invalid"
to maybe:
*  log once as VFS and then log any future ones as FYI
* log once as VFS and then only make the others available via dynamic
trace points
* rate limit it so we only log it once every n minutes?  (this is overkill?)



>
> Also, from the MS-SMB2 documentation:
> https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/4f35576a-6f3b-40f0-a832-1c30b0afccb3
>
> "The client MUST locate the file in the GlobalFileTable using the
> LeaseKey in the Lease Break Notification. If a file is not found, no
> further processing is required."
>
> So I don't think we should be changing the logic here.
> If SMB v1 had a different behaviour, we should check if that is as per
> the protocol documentation. If not, change it.
>
> Regards,
> Shyam
>
> On Fri, Mar 5, 2021 at 3:12 PM Vincent Whitchurch
>  wrote:
> >
> > The SMB1 version of ->is_oplock_break() returns true even if the FileId
> > is not found, as long as the oplock break notification message structure
> > itself appears to be valid.  A true return value makes
> > cifs_demultiplex_thread() to not print an error message for such
> > packets.
> >
> > However, the SMB2 version returns false in such cases, leading to an
> > error "No task to wake, unknown frame received!" followed by a hexdump
> > of the packet header being printed by cifs_demultiplex_thread().
> >
> > Note that before commit fa9c2362497fbd64788063288d ("CIFS: Fix SMB2
> > oplock break processing"), SMB2 also returned true for the case where a
> > connection was found but the FileId was not, but it's not clear to me if
> > that commit really intended to change the behaviour of the error prints.
> >
> > Change the behaviour of SMB2 to be the same as SMB1 and avoid the error
> > messages for these packets which we ignore as per the spec.
> >
> > Signed-off-by: Vincent Whitchurch 
> > ---
> >  fs/cifs/smb2misc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> > index 60d4bd1eae2b..3ea3bda64083 100644
> > --- a/fs/cifs/smb2misc.c
> > +++ b/fs/cifs/smb2misc.c
> > @@ -679,7 +679,7 @@ smb2_is_valid_lease_break(char *buffer)
> > }
> > spin_unlock(_tcp_ses_lock);
> > cifs_dbg(FYI, "Can not process lease break - no lease matched\n");
> > -   return false;
> > +   return true;
> >  }
> >
> >  bool
> > @@ -755,7 +755,7 @@ smb2_is_valid_oplock_break(char *buffer, struct 
> > TCP_Server_Info *server)
> > }
> > spin_unlock(_tcp_ses_lock);
> > cifs_dbg(FYI, "Can not process oplock break for non-existent 
> > connection\n");
> > -   return false;
> > +   return true;
> >  }
> >
> >  void
> > --
> > 2.28.0
> >
>
>
> --
> Regards,
> Shyam
>