| Message ID | 20230310153211.10982-9-sprasad@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] cifs: fix tcon status change after tree connect | expand |
Hi Shyam,
I love your patch! Perhaps something to improve:
[auto build test WARNING on cifs/for-next]
[also build test WARNING on linus/master v6.3-rc2 next-20230310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shyam-Prasad-N/cifs-generate-signkey-for-the-channel-that-s-reconnecting/20230310-234711
base: git://git.samba.org/sfrench/cifs-2.6.git for-next
patch link: https://lore.kernel.org/r/20230310153211.10982-9-sprasad%40microsoft.com
patch subject: [PATCH 09/11] cifs: account for primary channel in the interface list
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20230313/202303131349.VOlTuw2U-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303131349.VOlTuw2U-lkp@intel.com/
New smatch warnings:
fs/cifs/sess.c:366 cifs_chan_update_iface() warn: unsigned '--old_iface->weight_fulfilled' is never less than zero.
Old smatch warnings:
fs/cifs/sess.c:377 cifs_chan_update_iface() warn: unsigned '--old_iface->weight_fulfilled' is never less than zero.
vim +366 fs/cifs/sess.c
276
277 /*
278 * update the iface for the channel if necessary.
279 * will return 0 when iface is updated, 1 if removed, 2 otherwise
280 * Must be called with chan_lock held.
281 */
282 int
283 cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
284 {
285 unsigned int chan_index;
286 size_t iface_weight = 0, iface_min_speed = 0;
287 struct cifs_server_iface *iface = NULL;
288 struct cifs_server_iface *old_iface = NULL;
289 struct cifs_server_iface *last_iface = NULL;
290 int rc = 0;
291
292 spin_lock(&ses->chan_lock);
293 chan_index = cifs_ses_get_chan_index(ses, server);
294 if (ses->chans[chan_index].iface) {
295 old_iface = ses->chans[chan_index].iface;
296 if (old_iface->is_active) {
297 spin_unlock(&ses->chan_lock);
298 return 1;
299 }
300 }
301 spin_unlock(&ses->chan_lock);
302
303 spin_lock(&ses->iface_lock);
304 if (!ses->iface_count) {
305 spin_unlock(&ses->iface_lock);
306 cifs_dbg(VFS, "server %s does not advertise interfaces\n", ses->server->hostname);
307 return 0;
308 }
309
310 last_iface = list_last_entry(&ses->iface_list, struct cifs_server_iface,
311 iface_head);
312 iface_min_speed = last_iface->speed;
313
314 /* then look for a new one */
315 list_for_each_entry(iface, &ses->iface_list, iface_head) {
316 if (!chan_index) {
317 /* if we're trying to get the updated iface for primary channel */
318 if (!cifs_match_ipaddr((struct sockaddr *) &server->dstaddr,
319 (struct sockaddr *) &iface->sockaddr))
320 continue;
321
322 kref_get(&iface->refcount);
323 break;
324 }
325
326 /* do not mix rdma and non-rdma interfaces */
327 if (iface->rdma_capable != server->rdma)
328 continue;
329
330 if (!iface->is_active ||
331 (is_ses_using_iface(ses, iface) &&
332 !iface->rss_capable)) {
333 continue;
334 }
335
336 /* check if we already allocated enough channels */
337 iface_weight = iface->speed / iface_min_speed;
338
339 if (iface->weight_fulfilled >= iface_weight)
340 continue;
341
342 kref_get(&iface->refcount);
343 break;
344 }
345
346 if (list_entry_is_head(iface, &ses->iface_list, iface_head)) {
347 rc = 1;
348 iface = NULL;
349 cifs_dbg(FYI, "unable to find a suitable iface\n");
350 }
351
352 if (!chan_index && !iface) {
353 cifs_dbg(VFS, "unable to get the interface matching: %pIS\n",
354 &server->dstaddr);
355 spin_unlock(&ses->iface_lock);
356 return 0;
357 }
358
359 /* now drop the ref to the current iface */
360 if (old_iface && iface) {
361 cifs_dbg(FYI, "replacing iface: %pIS with %pIS\n",
362 &old_iface->sockaddr,
363 &iface->sockaddr);
364
365 old_iface->num_channels--;
> 366 if (--old_iface->weight_fulfilled < 0)
367 old_iface->weight_fulfilled = 0;
368 iface->num_channels++;
369 iface->weight_fulfilled++;
370
371 kref_put(&old_iface->refcount, release_iface);
372 } else if (old_iface) {
373 cifs_dbg(FYI, "releasing ref to iface: %pIS\n",
374 &old_iface->sockaddr);
375
376 old_iface->num_channels--;
377 if (--old_iface->weight_fulfilled < 0)
378 old_iface->weight_fulfilled = 0;
379
380 kref_put(&old_iface->refcount, release_iface);
381 } else if (!chan_index) {
382 /* special case: update interface for primary channel */
383 cifs_dbg(FYI, "referencing primary channel iface: %pIS\n",
384 &iface->sockaddr);
385 iface->num_channels++;
386 iface->weight_fulfilled++;
387 } else {
388 WARN_ON(!iface);
389 cifs_dbg(FYI, "adding new iface: %pIS\n", &iface->sockaddr);
390 }
391 spin_unlock(&ses->iface_lock);
392
393 spin_lock(&ses->chan_lock);
394 chan_index = cifs_ses_get_chan_index(ses, server);
395 ses->chans[chan_index].iface = iface;
396
397 /* No iface is found. if secondary chan, drop connection */
398 if (!iface && CIFS_SERVER_IS_CHAN(server))
399 ses->chans[chan_index].server = NULL;
400
401 spin_unlock(&ses->chan_lock);
402
403 if (!iface && CIFS_SERVER_IS_CHAN(server))
404 cifs_put_tcp_session(server, false);
405
406 return rc;
407 }
408
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index 78a7cfa75e91..9b51b2309e9c 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -291,11 +291,6 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server) spin_lock(&ses->chan_lock); chan_index = cifs_ses_get_chan_index(ses, server); - if (!chan_index) { - spin_unlock(&ses->chan_lock); - return 0; - } - if (ses->chans[chan_index].iface) { old_iface = ses->chans[chan_index].iface; if (old_iface->is_active) { @@ -318,6 +313,16 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server) /* then look for a new one */ list_for_each_entry(iface, &ses->iface_list, iface_head) { + if (!chan_index) { + /* if we're trying to get the updated iface for primary channel */ + if (!cifs_match_ipaddr((struct sockaddr *) &server->dstaddr, + (struct sockaddr *) &iface->sockaddr)) + continue; + + kref_get(&iface->refcount); + break; + } + /* do not mix rdma and non-rdma interfaces */ if (iface->rdma_capable != server->rdma) continue; @@ -344,16 +349,41 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server) cifs_dbg(FYI, "unable to find a suitable iface\n"); } + if (!chan_index && !iface) { + cifs_dbg(VFS, "unable to get the interface matching: %pIS\n", + &server->dstaddr); + spin_unlock(&ses->iface_lock); + return 0; + } + /* now drop the ref to the current iface */ if (old_iface && iface) { cifs_dbg(FYI, "replacing iface: %pIS with %pIS\n", &old_iface->sockaddr, &iface->sockaddr); + + old_iface->num_channels--; + if (--old_iface->weight_fulfilled < 0) + old_iface->weight_fulfilled = 0; + iface->num_channels++; + iface->weight_fulfilled++; + kref_put(&old_iface->refcount, release_iface); } else if (old_iface) { cifs_dbg(FYI, "releasing ref to iface: %pIS\n", &old_iface->sockaddr); + + old_iface->num_channels--; + if (--old_iface->weight_fulfilled < 0) + old_iface->weight_fulfilled = 0; + kref_put(&old_iface->refcount, release_iface); + } else if (!chan_index) { + /* special case: update interface for primary channel */ + cifs_dbg(FYI, "referencing primary channel iface: %pIS\n", + &iface->sockaddr); + iface->num_channels++; + iface->weight_fulfilled++; } else { WARN_ON(!iface); cifs_dbg(FYI, "adding new iface: %pIS\n", &iface->sockaddr); diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index c342a1db33ed..a5e53cb1ac49 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -740,6 +740,7 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_ unsigned int ret_data_len = 0; struct network_interface_info_ioctl_rsp *out_buf = NULL; struct cifs_ses *ses = tcon->ses; + struct TCP_Server_Info *pserver; /* do not query too frequently */ if (ses->iface_last_update && @@ -764,6 +765,11 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_ if (rc) goto out; + /* check if iface is still active */ + pserver = ses->chans[0].server; + if (pserver && !cifs_chan_is_iface_active(ses, pserver)) + cifs_chan_update_iface(ses, pserver); + out: kfree(out_buf); return rc;
The refcounting of server interfaces should account for the primary channel too. Although this is not strictly necessary, doing so will account for the primary channel in DebugData. Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> --- fs/cifs/sess.c | 40 +++++++++++++++++++++++++++++++++++----- fs/cifs/smb2ops.c | 6 ++++++ 2 files changed, 41 insertions(+), 5 deletions(-)