Bug 7478 - nanosleep used unconditionally in .../source3/modules/vfs_scannedonly.c
nanosleep used unconditionally in .../source3/modules/vfs_scannedonly.c
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: Build environment
3.5.6
Other Other
: P3 normal
: ---
Assigned To: Olivier Sessink
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-28 07:36 UTC by Joachim Schmitz
Modified: 2012-05-30 12:12 UTC (History)
2 users (show)

See Also:
metze: review+


Attachments
patch (956 bytes, patch)
2010-10-23 08:44 UTC, Joachim Schmitz
no flags Details
patch (950 bytes, patch)
2010-10-24 03:17 UTC, Joachim Schmitz
no flags Details
remove nanosleep and improve the whole wait loop behavior (4.38 KB, patch)
2010-11-13 09:34 UTC, Olivier Sessink
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joachim Schmitz 2010-05-28 07:36:37 UTC
nanosleep used unconditionally in .../source3/modules/vfs_scannedonly.c

Possible (but stupid) Patch:

diff -u ./source3/modules/vfs_scannedonly.c.orig ./source3/modules/vfs_scannedonly.c
--- ./source3/modules/vfs_scannedonly.c.orig    2010-05-17 06:51:23.000000000 -0500
+++ ./source3/modules/vfs_scannedonly.c 2010-05-27 09:27:33.000000000 -0500
@@ -476,7 +476,9 @@
                               "(max %d), %d ms) for %s\n",
                               i, recheck_tries,
                               recheck_time, cache_smb_fname->base_name));
+#ifdef HAVE_NANOSLEEP /* ? see also lib/util.c, function smb_msleep() */
                        nanosleep(&req, NULL);
+#endif
                        retval = SMB_VFS_NEXT_STAT(handle, cache_smb_fname);
                        i++;
                }


As mentioned above the real solution might be found in ...lib/util.c, function smb_msleep() 

Bye, Jojo
Comment 1 Björn Jacke 2010-09-16 15:00:16 UTC
thanks! fixed in master 3.6 with ec643df212e521fc19119820b1e4fac15986bf28. 
I'll close this for now. Joachim: if you need this fixed in 3.5, please reopen this bug.
Comment 2 Joachim Schmitz 2010-10-23 08:44:05 UTC
Created attachment 6029 [details]
patch
Comment 3 Joachim Schmitz 2010-10-23 08:44:30 UTC
Yes, I'd need it for 3.5.x too.

For now I've come up with the following patch, not realy sure whether it'd work though?

diff -u ./source3/modules/vfs_scannedonly.c.orig ./source3/modules/vfs_scannedonly.c
--- ./source3/modules/vfs_scannedonly.c.orig    2010-10-07 11:41:16.000000000 -0500
+++ ./source3/modules/vfs_scannedonly.c 2010-10-23 08:08:46.000000000 -0500
@@ -476,13 +476,21 @@
                flush_sendbuffer(handle);
                while (retval != 0      /*&& errno == ENOENT */
                       && i < recheck_tries) {
+#ifdef HAVE_NANOSLEEP
                        struct timespec req = { 0, recheck_time * 10000 };
+#endif
                        DEBUG(SCANNEDONLY_DEBUG,
                              ("scannedonly_allow_access, wait (try=%d "
                               "(max %d), %d ms) for %s\n",
                               i, recheck_tries,
                               recheck_time, cache_smb_fname->base_name));
+#ifdef HAVE_NANOSLEEP
                        nanosleep(&req, NULL);
+#elif defined HAVE_USLEEP
+                       usleep(recheck_time/1000);
+#else /* fallback, see also lib/util.c, function smb_msleep() */
+                       smb_msleep(recheck_time/1000000);
+#endif
                        retval = SMB_VFS_NEXT_STAT(handle, cache_smb_fname);
                        i++;
                }
Comment 4 Joachim Schmitz 2010-10-23 09:01:33 UTC
See also Bug 7464, which has struct timespec at the wrong place. It may not be needed at all if we don't have nanosleep()?
Comment 5 Joachim Schmitz 2010-10-24 03:15:31 UTC
Sorry this was nonsense, here a correction:

diff -u ./source3/modules/vfs_scannedonly.c.orig
./source3/modules/vfs_scannedonly.c
--- ./source3/modules/vfs_scannedonly.c.orig    2010-10-07 11:41:16.000000000
-0500
+++ ./source3/modules/vfs_scannedonly.c 2010-10-23 08:08:46.000000000 -0500
@@ -476,13 +476,21 @@
                flush_sendbuffer(handle);
                while (retval != 0      /*&& errno == ENOENT */
                       && i < recheck_tries) {
+#ifdef HAVE_NANOSLEEP
                        struct timespec req = { 0, recheck_time * 10000 };
+#endif
                        DEBUG(SCANNEDONLY_DEBUG,
                              ("scannedonly_allow_access, wait (try=%d "
                               "(max %d), %d ms) for %s\n",
                               i, recheck_tries,
                               recheck_time, cache_smb_fname->base_name));
+#ifdef HAVE_NANOSLEEP
                        nanosleep(&req, NULL);
+#elif defined HAVE_USLEEP
+                       usleep(recheck_time*10);
+#else /* fallback, see also lib/util.c, function smb_msleep() */
+                       smb_msleep(recheck_time/100);
+#endif
                        retval = SMB_VFS_NEXT_STAT(handle, cache_smb_fname);
                        i++;
                }
Comment 6 Joachim Schmitz 2010-10-24 03:17:26 UTC
Created attachment 6030 [details]
patch
Comment 7 Stefan Metzmacher 2010-10-24 05:48:55 UTC
Why not the same patch as commit ec643df212e521fc19119820b1e4fac15986bf28 ?

Adding ifdef's is really ugly.
Comment 8 Joachim Schmitz 2010-10-24 06:17:23 UTC
(In reply to comment #7)
> Why not the same patch as commit ec643df212e521fc19119820b1e4fac15986bf28 ?

How does that look?
Comment 9 Björn Jacke 2010-10-25 15:01:40 UTC
so:

http://git.samba.org/samba.git/?p=samba.git;a=commitdiff;h=ec643df212e521fc19119820b1e4fac15986bf28

that cherry-picks cleanly to 3.5. metze: can you give a quit ACK for a git cherry-pick to 3.5 and reassign to Karolin then?
Comment 10 Stefan Metzmacher 2010-10-25 16:55:27 UTC
Looks, ok if it applies to 3.5.x

Karolin please pick that one.
Comment 11 Joachim Schmitz 2010-10-25 23:12:46 UTC
(In reply to comment #9)
> so:
> http://git.samba.org/samba.git/?p=samba.git;a=commitdiff;h=ec643df212e521fc19119820b1e4fac15986bf28

Ah, thanks. Nice and simple. I searched for this but couldn't find it.

I assume it doesn't matter that this makes it sleep 2 orders of magnitudes longer? 'recheck_time*10000' nanoseconds would be 'recheck_time/100' miliseconds... But then again this might be 0 and not sleep at all.
Comment 12 Björn Jacke 2010-11-08 16:31:00 UTC
the change of factor 100 was not really intended but seemed to be reasonable as the documentation just mentions ms values. Reading the loop around the smb_msleep call (and from the man page) it also looks like smb_msleep(recheck_time) is correct.

I'm reopening this bug and reassign it to Oliver, the author of that module to confirm that ec643df212e introduced the correct sleep time here.
Comment 13 Olivier Sessink 2010-11-10 01:43:18 UTC
smb_msleep() sleeps milliseconds? I'm a bit puzzled by my own code, the recheck_time is supposed to me in milliseconds, but recheck_time*10000 doesn't make milliseconds from nanoseconds, there is a factor 100 missing. So in the code it has been milliseconds/100, or microseconds*10. 

anyway: the default values for recheck_time are 50 and 100

100/100 = 1 
50/100 might become 0 ....

perhaps a MAX(1, recheck_time/100) might be better. I'll do some tests to see if this has the desired result.
Comment 14 Björn Jacke 2010-11-10 03:30:44 UTC
as you say, the documentation says milliseconds: "...will wait recheck_tries_open times for recheck_time_open milliseconds...". I think the documentation would match the current code here now. But you should know best what the desired result is. If there is anything that should be changed, can you submit a patch for it, please? Otherwise if you think it's right now, you can close this bug again :-)
Comment 15 Olivier Sessink 2010-11-10 10:54:05 UTC
the smb_usleep() works fine, but I've improved the behavior a bit. Would you like that attached to this bug report, or as a separate bugreport?
Comment 16 Olivier Sessink 2010-11-13 09:34:33 UTC
Created attachment 6067 [details]
remove nanosleep and improve the whole wait loop behavior

this patch probably only applies after https://bugzilla.samba.org/show_bug.cgi?id=7789 has been committed
Comment 17 Björn Jacke 2010-11-23 16:02:45 UTC
Olivier: which branch is this patch for? In master and v3-5-test the nanosleep call is already eliminated and the sleep time is in units of ms there. So your patch doesn't apply.

If the latest changes in master and v3-5-test look sane for you then please close this bug.

Any further improvement should be put into another bug report or if it's just improvemnts that is not fixing bugs, you can send it directly to samba-techical for example.
Comment 18 Stefan Metzmacher 2012-05-30 12:12:44 UTC
The patches are in current branches