Bug 9870 - Not fetching the latest modification time on a folder if we have read locks on it.
Not fetching the latest modification time on a folder if we have read locks o...
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: SMB2
3.6.12
All FreeBSD
: P5 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2013-05-08 00:44 UTC by Hemanth
Modified: 2015-01-09 10:32 UTC (History)
3 users (show)

See Also:


Attachments
smb2 torture test case for modtime. (2.73 KB, text/plain)
2013-05-08 00:44 UTC, Hemanth
no flags Details
Patch for write time update. (743 bytes, patch)
2013-05-08 18:17 UTC, Hemanth
no flags Details
Test patch (not git format). (479 bytes, patch)
2013-12-03 23:28 UTC, Jeremy Allison
no flags Details
git-am patch I've proposed for master. (6.16 KB, patch)
2013-12-04 18:45 UTC, Jeremy Allison
no flags Details
git-am fix for 4.1.next and 4.0.next. (7.08 KB, patch)
2013-12-06 18:31 UTC, Jeremy Allison
asn: review+
jra: review? (ddiss)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hemanth 2013-05-08 00:44:14 UTC
Created attachment 8870 [details]
smb2 torture test case for modtime.

FreeBSD 8.0
Samba 3.6.12

Created a subdir/file inside a parent folder. At this time parent folder's modtime will be updated. Looks like samba is still showing the old modtime.

Whereas I am able to see the latest timestamp on the file system(shell). 
Looks like Samba is unable to list the latest modtime when it has some read locks( can hold read lock on opening the folder in explorer) on the folder.

I have written a torture test case to demonstrate the same. 

- Basically created a directory and opened for read.
- Created a sub-dir inside the parent.
- Checked the modtime and changetime values against the subdir's created in step 2.

[root@cent-57 bin]# ./smbtorture //xx.xx.xx.xx/share1 smb2.create.mod_time_check -t samba3  -U <domain>\\administrator%password -S required 
smbtorture 4.1.0
Using seed 1367968584
time: 2013-05-07 16:16:24.097508
test: mod_time_check
time: 2013-05-07 16:16:24.098665
TESTING CHANGE TIME ON DIRECTORIES
OPENING THE PARENT DIRECTORY
CREATING SUBDIR INSIDE PARENT
NOW CHECK THE TIMES ON PARENT:IT SHOULD MATCH WITH SUBDIR's
        create_time:    Tue May  7 15:57:16 2013 PDT
        access_time:    Tue May  7 15:57:16 2013 PDT
        write_time:     Tue May  7 16:05:20 2013 PDT
        change_time:    Tue May  7 16:05:20 2013 PDT
        attrib:         0x10
        alloc_size:     0
        size:           0
        nlink:          1
        delete_pending: 0
        directory:      1
        ea_size:        57504
        fname:          '(null)'
WARNING!: (../source4/torture/smb2/create.c:1549) wrong time for field write_time  Tue May  7 16:13:43 2013 PDT - Tue May  7 16:05:20 2013 PDT

        create_time:    Tue May  7 15:57:16 2013 PDT
        access_time:    Tue May  7 15:57:16 2013 PDT
        write_time:     Tue May  7 16:05:20 2013 PDT
        change_time:    Tue May  7 16:05:20 2013 PDT
        attrib:         0x10
        alloc_size:     0
        size:           0
        nlink:          1
        delete_pending: 0
        directory:      1
        ea_size:        57504
        fname:          '(null)'
time: 2013-05-07 16:16:24.140613
failure: mod_time_check [
(../source4/torture/smb2/create.c:1550) wrong time for field change_time  Tue May  7 16:13:43 2013 PDT - Tue May  7 16:05:20 2013 PDT ]

This is getting passed with Windows.

[root@cent-57 bin]# ./smbtorture //xx.xxx.xx.xx/hemanth smb2.create.mod_time_check -t samba3  -U <domain>\\administrator%password -S required smbtorture 4.1.0
Using seed 1367968601
time: 2013-05-07 16:16:41.827484
test: mod_time_check
time: 2013-05-07 16:16:41.828653
TESTING CHANGE TIME ON DIRECTORIES
OPENING THE PARENT DIRECTORY
CREATING SUBDIR INSIDE PARENT
NOW CHECK THE TIMES ON PARENT:IT SHOULD MATCH WITH SUBDIR's
time: 2013-05-07 16:16:41.839034
success: mod_time_check

Will be attaching the torture test ran along this bug.
Comment 1 Richard Sharpe 2013-05-08 03:04:54 UTC
Jeremy,

This is the case I mentioned in email.
Comment 2 Hemanth 2013-05-08 07:31:16 UTC
Jeremy,

Looks like I have found the root cause.

In smbd_smb2_getinfo_send() after reading the file stat information, we are calling get_file_infos(). In this call we are basically trying to fetch the shared mode lock entry of the file/directory and storing the mtime as part of that entry.

get_file_infos(struct file_id id,
		    uint32_t name_hash,
		    bool *delete_on_close,
		    struct timespec *write_time)
{
	struct share_mode_lock *lck;

	if (delete_on_close) {
		*delete_on_close = false;
	}

	if (write_time) {
		ZERO_STRUCTP(write_time);
	}

	if (!(lck = fetch_share_mode_unlocked(talloc_tos(), id))) {
		return;
	}

	if (delete_on_close) {
		*delete_on_close = is_delete_on_close_set(lck, name_hash);
	}

	if (write_time) {
		struct timespec wt;

		wt = lck->changed_write_time;
		if (null_timespec(wt)) {
			wt = lck->old_write_time;
		}

		*write_time = wt;
	}

	TALLOC_FREE(lck);
}


And in smbd_do_qfilepathinfo() we have this check..

if (!null_timespec(write_time_ts) && !INFO_LEVEL_IS_UNIX(info_level)) {
		update_stat_ex_mtime(psbuf, write_time_ts);
	}

This check is getting passed and we end up updating the mtime with the write_time captured as part of get_file_infos(). Avoiding this check resolves the issue. But I am trying to understand the reason behind storing the old mtime  as part of get_file_infos().
Comment 3 Stefan Metzmacher 2013-05-08 10:53:30 UTC
The problem is that we don't call trigger_write_time_update*()
on the parent when creating files or subdirectories.

Maybe it's better to ignore the old write time from the locking.tdb
for directories.
Comment 4 Hemanth 2013-05-08 18:17:40 UTC
Created attachment 8872 [details]
Patch for write time update.

Avoiding this check for directories seems good idea. Based on this I have actually modified update_stat_ex_mtime() to have this additional dir check. 

Attached the patch for the same. 

Please let me know if the patch looks fine.
Comment 5 Jeremy Allison 2013-12-03 23:28:46 UTC
Created attachment 9501 [details]
Test patch (not git format).

Can you try the following patch ? I think it's a cleaner and smaller fix, in that it simply means we don't store any old timestamp on open directory handles, which stops the "old timestamp hanging around" problem.

I'll write a torture test to check this and if it passes then I'll submit to master.

Jeremy.
Comment 6 Hemanth 2013-12-04 07:14:20 UTC
(In reply to comment #5)
> Created attachment 9501 [details]
> Test patch (not git format).
> 
> Can you try the following patch ? I think it's a cleaner and smaller fix, in
> that it simply means we don't store any old timestamp on open directory
> handles, which stops the "old timestamp hanging around" problem.
> 
> I'll write a torture test to check this and if it passes then I'll submit to
> master.
> 
> Jeremy.

Jeremy,

I have tested this patch. It is working fine. 

Thanks,
Hemanth.
Comment 7 Jeremy Allison 2013-12-04 18:45:19 UTC
Created attachment 9502 [details]
git-am patch I've proposed for master.

Formal patch I've proposed for master, including test case.

Jeremy.
Comment 8 Jeremy Allison 2013-12-06 18:31:28 UTC
Created attachment 9513 [details]
git-am fix for 4.1.next and 4.0.next.

Updated with cherry-pick information from master.

Patches apply cleanly to 4.1.x, 4.0.x.

Jeremy.
Comment 9 Karolin Seeger 2013-12-10 15:53:48 UTC
(In reply to comment #8)
> Created attachment 9513 [details]
> git-am fix for 4.1.next and 4.0.next.
> 
> Updated with cherry-pick information from master.
> 
> Patches apply cleanly to 4.1.x, 4.0.x.
> 
> Jeremy.

Patch seems to be reviewed. Can I pick it for 4.1 and 4.0?
Comment 10 Jeremy Allison 2013-12-10 17:17:34 UTC
Yes please !

Re-assigning to Karolin for inclusion in 4.0.next, 4.1.next.

Jeremy.
Comment 11 Karolin Seeger 2013-12-10 17:22:55 UTC
Pushed to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 12 Karolin Seeger 2013-12-14 19:26:36 UTC
Pushed to v4-1-test and v4-0-test.
Closing out bug report.

Thanks!