Ticket #3097 (closed defect: fixed)

Opened 6 years ago

Last modified 5 years ago

[patch] Recursive find file does not search recursively on samba share

Reported by: bilbo Owned by: andrew_b
Priority: major Milestone: 4.8.13
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset:

Description

When searching recursively a samba share, mc does not go into any subdirectories even if "Find recursively" is checked.

With a bit of debugging I found a cause:

In src/filemanager/file.c in function do_search, the algorithm executes mc_stat on a file (directory) and checks number of links. It assumes number of links minus 2 is number of subdirectories available for recursive search. While that assumption is valid on probably all linux filesystems, it is not valid on mounted samba shares. For these, number of links (as reported by stat or seen by 'ls -l') is not 1 for most files and 2+ for directories, but zero for all files and directories within the share (I guess that is how the kenrel samba module works ...). Therefore the optimization (that may limit the recursion) done by subdirs_left breaks the recursive search.

This error is in current master branch.

I am attaching a patch that fixes the issue (basically removes the subdirs_left optimization that is broken)

The patch seems not to have any side effects, the search works fine on ext4, vfat and samba share.

Attachments

fix_find_recursive.patch (1.8 KB) - added by bilbo 6 years ago.
Patch to fix recursive search on samba shares

Change History

Changed 6 years ago by bilbo

Patch to fix recursive search on samba shares

comment:1 Changed 6 years ago by bilbo

  • Summary changed from Recursive find file does not search recursively on samba share to [patch] Recursive find file does not search recursively on samba share

comment:2 Changed 5 years ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Milestone changed from Future Releases to 4.8.13

comment:3 Changed 5 years ago by andrew_b

  • Blocking 3202 added
  • Branch state changed from no branch to on review

Branch: 3097_smb_recursive_find
changeset:3324ed0727042c5b8b62dc26a3fd72cf2f4e1fb0

comment:4 Changed 5 years ago by andrew_b

  • Status changed from accepted to testing
  • Resolution set to fixed
  • Blocking 3202 removed
  • Branch state changed from on review to merged

comment:5 Changed 5 years ago by andrew_b

  • Status changed from testing to closed

comment:6 follow-up: ↓ 7 Changed 5 years ago by ossi

ehm ... you fixed it by completely removing the optimization, i.e., pessimized it for everybody because there are some broken systems. interesting approach ...

for samba, it could have been trivially fixed by disabling it only when a directory's initial ref count it zero. i presume it would be the same for the windows-served nfs?

and ... review process? wasn't there something?

comment:7 in reply to: ↑ 6 Changed 5 years ago by andrew_b

Replying to ossi:

for samba, it could have been trivially fixed by disabling it only when a directory's initial ref count it zero.

What do you mean? As initially reported,

...number of links minus 2 is number of subdirectories available for recursive search. While that assumption is valid on probably all linux filesystems, it is not valid on mounted samba shares. For these, number of links (as reported by stat or seen by 'ls -l') is not 1 for most files and 2+ for directories, but zero for all files and directories within the share.

comment:8 follow-up: ↓ 9 Changed 5 years ago by ossi

hmm? what's so hard about "if it's zero, ignore it"?

comment:9 in reply to: ↑ 8 Changed 5 years ago by andrew_b

Replying to ossi:

hmm? what's so hard about "if it's zero, ignore it"?

Please show the your code.

About optimization. Previously, we called mc_lstat() before push_directory() and mc_stat() after pop_directory(). We called stat() twice for the same directory. Now we remove call of mc_stat(). I thing it is some compensation for the removed optimization.

And more. I mount native Windows share via cifs. 'ls -l' shows me 0 links for directory, mc_lstat() too, but mc_stat() gives me st_nlink == 1. As a result, subdirs_left is negative, and subdirs_left-- never comes to zero.

comment:10 Changed 5 years ago by ossi

i really don't see the challenge (i haven't looked very closely what obstacles the mc vfs implementation puts into the path, but the algorithm itself has been implemented successfully many times before).

anyway, this discussion is actually pointless, as the optimization is supposed to save stat()ing every found file, but we need to do that anyway.

on a completely unrelated matter, the glib2-based implementation of pop_directory does not match the hand-coded one, and it shows in the erratic directory search order. it should use g_queue_pop_head.

Note: See TracTickets for help on using tickets.