On Fri, May 30, 2014 at 10:49 AM, David Howells <[email protected]> wrote:
> J. R. Okajima <[email protected]> wrote:
>
>> I've found some interesting cases.
>>
>> - impermissible.test,
>> open_file_as_bin -t -w $file -E EACCES
>> When $termslash is "/", a '/' is appended to the expanded $file, such
>> as "/path/fileA/". If fileA is a regular file, that path is obviously
>> wrong. Does UnionMount expect EACCES in this case too?
>> Should it be ENOTDIR?
>> It might be better to change
>> errcode=EACCES
>> test ! "$termslash" = "" && errcode=ENOTDIR
>> open_file_as_bin -t -w $file -E $errcode
>
> I'd never got to the end of the impermissible test because the utimes test
> fails on both unionmount and overlayfs. I'll have to address the termslash
> alterations at some point.
>
[ Re-Tested with 3.15.0-rc8-1-iniza-lockdep ]
Running the impermissible test on OverlayFS with TERMSLASH=1 is
successful here (TERMSLASH=0 still fails):
# LC_ALL=C TEST_OVERLAYFS=1 TERMSLASH=1 ./run.sh
[ NOTE-1: I still see the reported call-trace which you Dave see as a
problem in OverlayFS. ]
[ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
[ ^^ Miklos any idea? ]
Diverse logs, kernel-config and dmesg files attached.
Hope this feedback helps you.
- Sedat -
Sedat Dilek <[email protected]> wrote:
> [ Re-Tested with 3.15.0-rc8-1-iniza-lockdep ]
>
> Running the impermissible test on OverlayFS with TERMSLASH=1 is
> successful here
That's not very surprising. utimensat() doesn't even get out of pathwalk if
the filename has a terminal slash and the fs_op macro correctly overrides the
expected error code in this case.
> (TERMSLASH=0 still fails):
It's possible that the test is wrong. utimes is icky, at least on unionmount,
because the copy up has to be done before we call notify_change() - but
notify_change() can still reject the operation (in which case we didn't want
to copy up at all). The problem is that notify_change() has i_mutex.
Ideally, we'd do copy-up inside notify_change() - but we can't do that in
unionmount. I imagine you can do that in overlayfs.
> # LC_ALL=C TEST_OVERLAYFS=1 TERMSLASH=1 ./run.sh
>
> [ NOTE-1: I still see the reported call-trace which you Dave see as a
> problem in OverlayFS. ]
There should be zero lockdep complaints. It may be a real locking problem or
it may just be a missing nesting annotation.
One problem I had with unionmounts that I don't think applies to overlayfs is
that i_{,mdir_}mutex on inodes on the upper (union) layer and the lower layers
may come from the same superblock and thus have the same lockdep class keys
(which are per struct file_system_type). This means that lockdep thinks
they're conflicting - when actually they are not. What I did was to give
i_{,dir_}mutex in the upper layer different lockdep classes.
The reason this works for unionmounts is that there unionmounting is done by
the VFS and the upper/glue layer is an ordinary superblock (tmpfs, ext2, ...)
and I get to meddle with the lockdep classes of the upper superblock during
mount before any inodes exist.
I don't think this would work for overlayfs for two reasons: (1) the top layer
is an overlayfs superblock and would have different lockdep classes anyway and
(2) you cannot change the lockdep class once inodes exist in the filesystem -
and overlayfs requires both the upper and lower layers to exist prior to the
glue layer being mounted.
My suspicion is that overlayfs is doing stuff to the lower layer whilst
holding a lock on the upper layer or vice versa.
> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
Do you know for which operation?
David
On Tue, Jun 3, 2014 at 11:00 AM, David Howells <[email protected]> wrote:
> My suspicion is that overlayfs is doing stuff to the lower layer whilst
> holding a lock on the upper layer or vice versa.
No. It's holding the overlayfs i_mutex and then getting either the
upper *or* the lower i_mutex, but never both. So it's a simple and
unproblematic ordering.
Thanks,
Miklos
[...]
>> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
>
> Do you know for which operation?
>
# echo $TESTS
open-plain.test open-trunc.test open-creat.test open-creat-trunc.test
open-creat-excl.test open-creat-excl-trunc.test noent-plain.test
noent-trunc.test noent-creat.test noent-creat-trunc.test
noent-creat-excl.test noent-creat-excl-trunc.test sym1-plain.test
sym1-trunc.test sym1-creat.test sym1-creat-excl.test sym2-plain.test
sym2-trunc.test sym2-creat.test sym2-creat-excl.test symx-plain.test
symx-trunc.test symx-creat.test symx-creat-excl.test
symx-creat-trunc.test truncate.test dir-open.test dir-weird-open.test
dir-open-dir.test dir-weird-open-dir.test dir-sym1-open.test
dir-sym1-weird-open.test dir-sym2-open.test dir-sym2-weird-open.test
readlink.test impermissible.test
# for i in $TESTS ; do echo [ $i ] ; LC_ALL=C TEST_OVERLAYFS=1
TERMSLASH=1 ./run.sh $i 2>&1 | tee ../run-sh-log_${i}.txt ; dmesg
>dmesg_${i}.txt ; done
# for i in $TESTS ; do echo [ $i ] ; grep "INFO: possible recursive
locking detected" dmesg_${i}.txt ; done
...
[ symx-creat-trunc.test ]
[ truncate.test ]
[ 1910.007203] [ INFO: possible recursive locking detected ]
...
LAST-GOOD: dmesg_symx-creat-trunc.test.txt
FIRST-BAD: dmesg_truncate.test.txt
dmesg files attached.
- Sedat -
On Tue, Jun 3, 2014 at 11:18 AM, Sedat Dilek <[email protected]> wrote:
> [...]
>>> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
>>
>> Do you know for which operation?
>>
>
> # echo $TESTS
> open-plain.test open-trunc.test open-creat.test open-creat-trunc.test
> open-creat-excl.test open-creat-excl-trunc.test noent-plain.test
> noent-trunc.test noent-creat.test noent-creat-trunc.test
> noent-creat-excl.test noent-creat-excl-trunc.test sym1-plain.test
> sym1-trunc.test sym1-creat.test sym1-creat-excl.test sym2-plain.test
> sym2-trunc.test sym2-creat.test sym2-creat-excl.test symx-plain.test
> symx-trunc.test symx-creat.test symx-creat-excl.test
> symx-creat-trunc.test truncate.test dir-open.test dir-weird-open.test
> dir-open-dir.test dir-weird-open-dir.test dir-sym1-open.test
> dir-sym1-weird-open.test dir-sym2-open.test dir-sym2-weird-open.test
> readlink.test impermissible.test
>
> # for i in $TESTS ; do echo [ $i ] ; LC_ALL=C TEST_OVERLAYFS=1
> TERMSLASH=1 ./run.sh $i 2>&1 | tee ../run-sh-log_${i}.txt ; dmesg
>>dmesg_${i}.txt ; done
>
> # for i in $TESTS ; do echo [ $i ] ; grep "INFO: possible recursive
> locking detected" dmesg_${i}.txt ; done
> ...
> [ symx-creat-trunc.test ]
> [ truncate.test ]
> [ 1910.007203] [ INFO: possible recursive locking detected ]
> ...
>
> LAST-GOOD: dmesg_symx-creat-trunc.test.txt
>
> FIRST-BAD: dmesg_truncate.test.txt
>
> dmesg files attached.
>
The lockdep appears one time in the logs... I tried several...
# LC_ALL=C TEST_OVERLAYFS=1 ./run.sh truncate.test
...and see the only lockdep.
Sorry, I cannot say which of the test-no (is that what you mean by
operation?) is causing the lockdep.
Truncate-test results attached.
- Sedat -
[...]
> The lockdep appears one time in the logs... I tried several...
>
> # LC_ALL=C TEST_OVERLAYFS=1 ./run.sh truncate.test
>
> ...and see the only lockdep.
>
> Sorry, I cannot say which of the test-no (is that what you mean by
> operation?) is causing the lockdep.
>
> Truncate-test results attached.
>
I have rebooted my machine and re-run truncate.test again.
# LC_ALL=C TEST_OVERLAYFS=1 ./run.sh truncate.test
So, the lockdep appears with the first operation...
***
*** TEST_OVERLAYFS=1 TERMSLASH=0 ./run.sh truncate.test
***
TEST129: Prepare comparison
16+0 records in
16+0 records out
TEST100: Truncate to 0
- fs_op truncate /mnt/a/foo100 0
1st FS-OP: XXX
[ 91.551101] [<ffffffff811dbeb5>] ? path_init+0x545/0x6a0
[ 91.551104] [<ffffffff810bc700>] ? mark_held_locks+0x90/0x90
[ 91.551107] [<ffffffff811dd0ab>] path_openat+0xbb/0x680
[ 91.551111] [<ffffffff811de6fa>] do_filp_open+0x3a/0x90
[ 91.551115] [<ffffffff8173a287>] ? _raw_spin_unlock+0x27/0x40
[ 91.551118] [<ffffffff811ec5b7>] ? __alloc_fd+0xa7/0x130
[ 91.551121] [<ffffffff811cc288>] do_sys_open+0x128/0x220
[ 91.551125] [<ffffffff81022805>] ? syscall_trace_enter+0x145/0x260
[ 91.551128] [<ffffffff811cc39e>] SyS_open+0x1e/0x20
[ 91.551132] [<ffffffff81744258>] tracesys+0xe1/0xe6
TEST101: Truncate to 1
...
The full log is attached.
- Sedat -
On Tue, Jun 3, 2014 at 11:26 AM, Sedat Dilek <[email protected]> wrote:
> On Tue, Jun 3, 2014 at 11:18 AM, Sedat Dilek <[email protected]> wrote:
>> [...]
>>>> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
>>>
>>> Do you know for which operation?
This still looks like the same annotation problem in
generic_file_splice_write() I already sent a patch for.
Fix now pushed to overlayfs.v22/overlayfs.current.
Also added ops to shmem to support the cases where RENAME_EXCHANGE and
RENAME_WHITEOUT are needed. Not sure if the testsuite goes into these
at all, but anyway.
If I remove the assert_early_copy_up calls from impermissible.test
(which seem to be bogus), then the suite passes for overlayfs without
lockdep problems for me.
Thanks,
Miklos
On Tue, Jun 3, 2014 at 11:42 AM, Miklos Szeredi <[email protected]> wrote:
> On Tue, Jun 3, 2014 at 11:26 AM, Sedat Dilek <[email protected]> wrote:
>> On Tue, Jun 3, 2014 at 11:18 AM, Sedat Dilek <[email protected]> wrote:
>>> [...]
>>>>> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
>>>>
>>>> Do you know for which operation?
>
> This still looks like the same annotation problem in
> generic_file_splice_write() I already sent a patch for.
>
> Fix now pushed to overlayfs.v22/overlayfs.current.
>
I have applied "vfs: fix wrong lockdep annotation in
generic_file_splice_write()" to my latest Linux-kernel and
truncate.test seems to be fine, now.
Feel free to add my Tested-by/Reported-by when you spinout a new
version of overlayfs in your Git tree.
For the sake of completeness my simple hack to identify the fs-op...
--- a/truncate.test
+++ b/truncate.test
@@ -33,6 +33,8 @@ for ((loop=0; loop<29; loop++)) {
fi
fs_op truncate $file $loop
+ echo "1st FS-OP: XXX"
+ dmesg | tail
assert_is_upper $file
post=`stat --printf %s $file`
@@ -48,5 +50,7 @@ for ((loop=0; loop<29; loop++)) {
fi
else
fs_op truncate $file $loop -E ENOTDIR
+ echo "2nd FS-OP: XXX"
+ dmesg | tail
fi
}
All tests now run fine!
# LC_ALL=C TEST_OVERLAYFS=1 TERMSLASH=1 ./run.sh
Thanks for all involved people.
- Sedat -
On Tue, Jun 3, 2014 at 11:42 AM, Miklos Szeredi <[email protected]> wrote:
> On Tue, Jun 3, 2014 at 11:26 AM, Sedat Dilek <[email protected]> wrote:
>> On Tue, Jun 3, 2014 at 11:18 AM, Sedat Dilek <[email protected]> wrote:
>>> [...]
>>>>> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
>>>>
>>>> Do you know for which operation?
>
> This still looks like the same annotation problem in
> generic_file_splice_write() I already sent a patch for.
>
> Fix now pushed to overlayfs.v22/overlayfs.current.
>
Will you send this patch to Al separately or will it go through your tree?
[ Do you happen to know if LTP has some tests especially to test pipes? ]
- Sedat -
Miklos Szeredi <[email protected]> wrote:
> Fix now pushed to overlayfs.v22/overlayfs.current.
I ran my testscript, which leaves a clean set up and mounted overlay fs
behind. I then ran:
for ((i=100; i<=129; i++)); do mv /mnt/a/foo$i /mnt/a/bar$i; done
for ((i=100; i<=129; i++)); do mv /mnt/a/dir$i /mnt/a/dir2$i; done
leading to:
=============================================
[ INFO: possible recursive locking detected ]
3.15.0-rc6-fsdevel+ #382 Tainted: G W
---------------------------------------------
mv/27935 is trying to acquire lock:
(&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff8111e555>] vfs_rmdir+0x59/0xe8
but task is already holding lock:
(&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff811e12a9>] ovl_clear_empty+0x175/0x1eb
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&sb->s_type->i_mutex_key#9);
lock(&sb->s_type->i_mutex_key#9);
*** DEADLOCK ***
May be due to missing lock nesting notation
5 locks held by mv/27935:
#0: (sb_writers#15){.+.+.+}, at: [<ffffffff8112c06c>] mnt_want_write+0x1c/0x40
#1: (&sb->s_type->i_mutex_key#17/1){+.+.+.}, at: [<ffffffff8111eb96>] do_rmdir+0x9f/0x152
#2: (&sb->s_type->i_mutex_key#17){+.+.+.}, at: [<ffffffff8111e555>] vfs_rmdir+0x59/0xe8
#3: (sb_writers#8){.+.+.+}, at: [<ffffffff8112c06c>] mnt_want_write+0x1c/0x40
#4: (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff811e12a9>] ovl_clear_empty+0x175/0x1eb
stack backtrace:
CPU: 1 PID: 27935 Comm: mv Tainted: G W 3.15.0-rc6-fsdevel+ #382
Hardware name: /DG965RY, BIOS MQ96510J.86A.0816.2006.0716.2308 07/16/2006
0000000000000000 ffff880038ac9af0 ffffffff8148b889 ffffffff81fda190
ffff880038ac9bb0 ffffffff81074b0e 0000000000000002 ffff88003823c890
0000000000000000 ffff880000000000 ffff880000000004 ffff880000000000
Call Trace:
[<ffffffff8148b889>] dump_stack+0x4d/0x66
[<ffffffff81074b0e>] __lock_acquire+0x75a/0x1861
[<ffffffff810762b6>] lock_acquire+0x9c/0x112
[<ffffffff8111e555>] ? vfs_rmdir+0x59/0xe8
[<ffffffff8148e6df>] mutex_lock_nested+0x60/0x2ff
[<ffffffff8111e555>] ? vfs_rmdir+0x59/0xe8
[<ffffffff8111e555>] vfs_rmdir+0x59/0xe8
[<ffffffff811e0fbe>] ovl_cleanup+0x1d/0x40
[<ffffffff811e12b4>] ovl_clear_empty+0x180/0x1eb
[<ffffffff811e1360>] ovl_check_empty_and_clear+0x41/0x5c
[<ffffffff81058fcc>] ? creds_are_invalid+0x18/0x45
[<ffffffff811e1aca>] ovl_do_remove+0x17c/0x35e
[<ffffffff811e1cbd>] ovl_rmdir+0x11/0x13
[<ffffffff8111e584>] vfs_rmdir+0x88/0xe8
[<ffffffff8111ebdd>] do_rmdir+0xe6/0x152
[<ffffffff810af5b4>] ? __audit_syscall_entry+0xa1/0xc3
[<ffffffff8100d737>] ? syscall_trace_enter+0x197/0x1eb
[<ffffffff8111f414>] SyS_unlinkat+0x16/0x29
[<ffffffff814925f4>] tracesys+0xdd/0xe2
On Tue, Jun 3, 2014 at 12:21 PM, Sedat Dilek <[email protected]> wrote:
> On Tue, Jun 3, 2014 at 11:42 AM, Miklos Szeredi <[email protected]> wrote:
>> On Tue, Jun 3, 2014 at 11:26 AM, Sedat Dilek <[email protected]> wrote:
>>> On Tue, Jun 3, 2014 at 11:18 AM, Sedat Dilek <[email protected]> wrote:
>>>> [...]
>>>>>> [ NOTE-2: The call-trace I have seen once (TERMSLASH=0). ]
>>>>>
>>>>> Do you know for which operation?
>>
>> This still looks like the same annotation problem in
>> generic_file_splice_write() I already sent a patch for.
>>
>> Fix now pushed to overlayfs.v22/overlayfs.current.
>>
>
> Will you send this patch to Al separately or will it go through your tree?
>
> [ Do you happen to know if LTP has some tests especially to test pipes? ]
>
Chris from LTP answered me to test with latest ltp-git [1] (here:
20140422-134-gd458410).
So, I have run IPC/Pipes/Syscalls tests...
# LC_ALL=C ./runltp -f ipc pipes syscalls
Test Start Time: Tue Jun 3 12:48:24 2014
-----------------------------------------
Testcase Result Exit Value
-------- ------ ----------
pipeio_1 PASS 0
pipeio_3 PASS 0
pipeio_4 PASS 0
pipeio_5 PASS 0
pipeio_6 PASS 0
pipeio_8 PASS 0
sem01 PASS 0
sem02 PASS 0
message_queue_test_01 PASS 0
message_queue_test_02_get PASS 0
message_queue_test_02_snd PASS 0
message_queue_test_02_rcv PASS 0
message_queue_test_02_ctl PASS 0
message_queue_test_04 PASS 0
message_queue_test_05 PASS 0
pipe_test_01 PASS 0
pipe_test_02 PASS 0
semaphore_test_01 PASS 0
semaphore_test_02 PASS 0
semaphore_test_03 PASS 0
shmem_test_01 PASS 0
shmem_test_02 PASS 0
shmem_test_03 PASS 0
shmem_test_04 PASS 0
shmem_test_05 PASS 0
shmem_test_06 PASS 0
shmem_test_07 PASS 0
signal_test_01 PASS 0
signal_test_02 PASS 0
signal_test_03 PASS 0
signal_test_04 PASS 0
signal_test_05 PASS 0
signal_test_06 PASS 0
signal_test_07 PASS 0
-----------------------------------------------
Total Tests: 34
Total Failures: 0
Kernel Version: 3.15.0-rc8-2-iniza-lockdep
Machine Architecture: x86_64
Hostname: fambox
So far no LOCKPDEPs in my logs.
- Sedat -
[1] http://marc.info/?l=ltp-list&m=140179189918510&w=2
[2] https://github.com/linux-test-project/ltp/blob/master/doc/mini-howto-building-ltp-from-git.txt
On Tue, Jun 03, 2014 at 11:33:54AM +0100, David Howells wrote:
> Miklos Szeredi <[email protected]> wrote:
>
> > Fix now pushed to overlayfs.v22/overlayfs.current.
>
> I ran my testscript, which leaves a clean set up and mounted overlay fs
> behind. I then ran:
>
> for ((i=100; i<=129; i++)); do mv /mnt/a/foo$i /mnt/a/bar$i; done
> for ((i=100; i<=129; i++)); do mv /mnt/a/dir$i /mnt/a/dir2$i; done
>
> leading to:
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.15.0-rc6-fsdevel+ #382 Tainted: G W
> ---------------------------------------------
> mv/27935 is trying to acquire lock:
> (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff8111e555>] vfs_rmdir+0x59/0xe8
>
> but task is already holding lock:
> (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff811e12a9>] ovl_clear_empty+0x175/0x1eb
And this one is a missing annotation in overlayfs. Tested patch pushed to the
usual branches.
Thanks,
Miklos
---
fs/overlayfs/dir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -253,7 +253,7 @@ static struct dentry *ovl_clear_empty(st
unlock_rename(workdir, upperdir);
ovl_cleanup_whiteouts(upper, list);
- mutex_lock(&wdir->i_mutex);
+ mutex_lock_nested(&wdir->i_mutex, I_MUTEX_PARENT);
ovl_cleanup(wdir, upper);
mutex_unlock(&wdir->i_mutex);
On Tue, Jun 3, 2014 at 3:21 PM, Miklos Szeredi <[email protected]> wrote:
> On Tue, Jun 03, 2014 at 11:33:54AM +0100, David Howells wrote:
>> Miklos Szeredi <[email protected]> wrote:
>>
>> > Fix now pushed to overlayfs.v22/overlayfs.current.
>>
>> I ran my testscript, which leaves a clean set up and mounted overlay fs
>> behind. I then ran:
>>
>> for ((i=100; i<=129; i++)); do mv /mnt/a/foo$i /mnt/a/bar$i; done
>> for ((i=100; i<=129; i++)); do mv /mnt/a/dir$i /mnt/a/dir2$i; done
>>
>> leading to:
>>
>> =============================================
>> [ INFO: possible recursive locking detected ]
>> 3.15.0-rc6-fsdevel+ #382 Tainted: G W
>> ---------------------------------------------
>> mv/27935 is trying to acquire lock:
>> (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff8111e555>] vfs_rmdir+0x59/0xe8
>>
>> but task is already holding lock:
>> (&sb->s_type->i_mutex_key#9){+.+.+.}, at: [<ffffffff811e12a9>] ovl_clear_empty+0x175/0x1eb
>
> And this one is a missing annotation in overlayfs. Tested patch pushed to the
> usual branches.
>
> Thanks,
> Miklos
>
> ---
> fs/overlayfs/dir.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -253,7 +253,7 @@ static struct dentry *ovl_clear_empty(st
>
> unlock_rename(workdir, upperdir);
> ovl_cleanup_whiteouts(upper, list);
> - mutex_lock(&wdir->i_mutex);
> + mutex_lock_nested(&wdir->i_mutex, I_MUTEX_PARENT);
> ovl_cleanup(wdir, upper);
> mutex_unlock(&wdir->i_mutex);
>
I have tested the latest overlayfs.current up to...
commit 17eb601eb5dbc8a2e200872380c03400813d4f1a
"overlayfs: annotate mutex in ovl_clear_empty()".
It looks good here.
- Sedat -
Miklos Szeredi <[email protected]> wrote:
> And this one is a missing annotation in overlayfs. Tested patch pushed to the
> usual branches.
Looks good so far, though there are a few more bits to try and break -
rename() for example.
I also want to rewrite my test stuff in something a little more wieldy than
shell scripts - Perl or Python for example.
Tentative Tested-by from me unless I can break it.
David
On Tue, Jun 3, 2014 at 5:30 PM, David Howells <[email protected]> wrote:
> Miklos Szeredi <[email protected]> wrote:
>
>> And this one is a missing annotation in overlayfs. Tested patch pushed to the
>> usual branches.
>
> Looks good so far, though there are a few more bits to try and break -
> rename() for example.
>
> I also want to rewrite my test stuff in something a little more wieldy than
> shell scripts - Perl or Python for example.
Shell script is the one I'm most familiar with, and that maybe true
for other kernel developers too.
Also, it's what xfstests are using and it would make sense to move
towards that. Although I'm not sure how well it supports
multiple-device filesystems.
>
> Tentative Tested-by from me unless I can break it.
Thanks for testing.
Miklos
Miklos Szeredi <[email protected]> wrote:
> Shell script is the one I'm most familiar with, and that maybe true
> for other kernel developers too.
>
> Also, it's what xfstests are using and it would make sense to move
> towards that. Although I'm not sure how well it supports
> multiple-device filesystems.
It was already shell script, but I can do all the layer checking and
automatically fixing up the error prediction for when pathnames have slashes
appended much more easily in something like python where I can build and
maintain a map of the state of the filesystem.
See:
http://git.infradead.org/users/dhowells/unionmount-testsuite.git/shortlog/refs/heads/switch-to-python
I've also added tests to exercise mkdir and rmdir and have enabled the unlink
tests. So far so good with overlayfs.
David
Miklos Szeredi <[email protected]> wrote:
> > Tentative Tested-by from me unless I can break it.
>
> Thanks for testing.
Tested-by retracted temporarily. There's a bug in rename handling. If you
clone:
http://git.infradead.org/users/dhowells/unionmount-testsuite.git
and check out the switch-to-python branch and do:
./run --no rename-mass
then this works with no unioning used, but if you do:
./run --ov rename-mass
to run it on overlayfs, then the following occurs:
[root@andromeda union-testsuite]# ./run --ov rename-mass
***
*** ./run --ov --ts=0 rename-mass
***
TEST rename-mass.py:15: Mass rename sequential files into each other's vacated name slots
./run --rename /mnt/a/foo103 /mnt/a/foo104
./run --rename /mnt/a/foo102 /mnt/a/foo103
./run --rename /mnt/a/foo101 /mnt/a/foo102
./run --rename /mnt/a/foo100 /mnt/a/foo101
./run --rename /mnt/a/foo104 /mnt/a/foo105
/mnt/a/foo104: File unexpectedly found
strace shows:
rename("/mnt/a/foo104", "/mnt/a/foo105") = 0
lstat("/mnt/a/foo104", {st_mode=S_IFREG|0644, st_size=12, ...}) = 0
which shouldn't happen.
The kernel is 17eb601eb5dbc8a2e200872380c03400813d4f1a from overlayfs.v22.
David
On Tue, Jun 24, 2014 at 05:46:14PM +0100, David Howells wrote:
> strace shows:
>
> rename("/mnt/a/foo104", "/mnt/a/foo105") = 0
> lstat("/mnt/a/foo104", {st_mode=S_IFREG|0644, st_size=12, ...}) = 0
>
> which shouldn't happen.
Sorry for the delay. Following patch fixes it and tests now run fine.
Problem was that copy-up didn't set opaque flag on non-dir. The reason this has
gone unnoticed is that the dentry on overlayfs was unhashed (to get rid of the
unneeded lower dentry reference) so after a new lookup the opaque flag would be
set correctly. Rename, however, rehashed the copied up dentry and so old_opaque
and the opaque flag on old would become out-of-sync.
As a followup patch we could also unhash the copied dentry after the rename, but
that's just an optimization.
Thanks for the report and the great test suite!
Miklos
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 1670dbe..274c857 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -255,9 +255,13 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
* Easiest way to get rid of the lower dentry reference is to
* drop this dentry. This is neither needed nor possible for
* directories.
+ *
+ * Non-directores become opaque when copied up.
*/
- if (!S_ISDIR(stat->mode))
+ if (!S_ISDIR(stat->mode)) {
+ ovl_dentry_set_opaque(dentry, true);
d_drop(dentry);
+ }
out:
dput(upper);
dput(newdentry);
Miklos Szeredi <[email protected]> wrote:
> Following patch fixes it and tests now run fine.
Yep, that seems to fix it. You can have my Tested-by back;-) Are you going
to release another branch in your git repo?
David
On Tue, Jul 8, 2014 at 11:56 AM, David Howells <[email protected]> wrote:
> Miklos Szeredi <[email protected]> wrote:
>
>> Following patch fixes it and tests now run fine.
>
> Yep, that seems to fix it. You can have my Tested-by back;-) Are you going
> to release another branch in your git repo?
Updated series pushed to overlayfs.v23 and overlayfs.current.
Thanks,
Miklos