Hello Steve, the goal of struct smb2_compound_op is to make it easier to work with compound requests. In particular the existing smb2_compound_op() function in smb2inode.c is very hard to understand, let alone modify. As an example it took me really a long time to get 64ce47cb1b29d7d6aab6 right, and I still see it as a very clumsy approach. It should not be neccesary to kmemdup output deep inside this routine, but it was the only approach I could find. To me it would be much easier if the open/qinfo/close sequence was flat in smb311_posix_query_path_info() and we could dissect the results while the response is still around. So yes, this fs/cifs/smb2compound.c is 200 lines of code, but I think it is really simply structured code that should be easy to review. Volker Am Sun, Mar 19, 2023 at 02:49:09PM -0500 schrieb Steve French: > My main worry is that it is relatively large (which can make it harder > to review, and harder for Ronnie and Paulo et al to backport) but if > it is the cleanest way to solve various problems, then it could be > worth pursuing. Otherwise, I prefer cleanup (unless relatively small > and reasonably easy to review) when it is related to fixing other > problems. > > The biggest problems I would like to fix are: > 1) using compounding in more places (plenty of benchmarks or simple > examples like gunzip, tar, rsync, to go through to look for places > where network traces show compounding not being used) > 2) optimizing compounding better e.g. > - "ls -l" of directory with mfsymlinks sends multiple compound > requests for every mfsymlink when only need one > - "ls" sends an extra roundtrip for 99% of cases, to send the > final querydir (which gets no more files rc ...). We should be sending > "open/querydir/querydir" compound not "open/querydir") > 3) ) we need to fix a few cases when we open a file in compound ops > when we already have an appropriate one open > 4) we request leases where appropriate for compounding sequences (e.g. > open/querydir) e.g. when open sent but no close and reasonable > possibility of benefiting from deferred close. > 5) we set the parent lease key where possible (quoting from MS-SMB2: > "The client MUST search the GlobalFileTable for the parent directory > of the file being opened. The name of the parent directory is obtained > by removing the last component of the path. If any entry is found, > ParentLeaseKey is obtained from File.LeaseKey of that entry and > SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET bit MUST be set in the Flags > field.") > 6) we need to make sure we are always requesting handle leases (e.g. > RWH or RH) - we have one case (cachiong directories) where we request > "R" instead of "RH" and we need to make sure that we close any > deferred opens (or cached dir opens) when we lose the 'H' on a lease > > > On Sun, Mar 19, 2023 at 2:19 PM Volker Lendecke > <Volker.Lendecke@xxxxxxxxx> wrote: > > > > Hello! > > > > This is not a formal patchset, more a request for comments. > > > > In fs/cifs when doing compound operations I see a lot of fiddly > > boilerplate code. Attached find a little patchset that introduces a > > struct smb2_compound_op capturing most of that boilerplate code, along > > with a few sample uses. > > > > Is that worth exploring further? > > > > Thanks, > > > > Volker > > > > -- > Thanks, > > Steve
