* [PATCH] ecryptfs: Fix inodes not being evicted until unmount
@ 2020-05-08 14:31 Jeffrey Mitchell
2020-05-08 14:32 ` Jeffrey Mitchell
0 siblings, 1 reply; 6+ messages in thread
From: Jeffrey Mitchell @ 2020-05-08 14:31 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, daniel.robertson
On asynchronous base filesystems like NFS, eCryptFS leaves inodes for
deleted files in the cache until unmounting. Change call in
ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably
evict inodes from the cache even on top of NFS.
Signed-off-by: Dan Robertson <daniel.robertson@starlab.io>
Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
---
fs/ecryptfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index e23752d..f7594b6 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
goto out_unlock;
}
fsstack_copy_attr_times(dir, lower_dir_inode);
- set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
+ drop_nlink(inode);
inode->i_ctime = dir->i_ctime;
out_unlock:
dput(lower_dentry);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
2020-05-08 14:31 [PATCH] ecryptfs: Fix inodes not being evicted until unmount Jeffrey Mitchell
@ 2020-05-08 14:32 ` Jeffrey Mitchell
0 siblings, 0 replies; 6+ messages in thread
From: Jeffrey Mitchell @ 2020-05-08 14:32 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, daniel.robertson
Steps to reproduce issue:
Mount nfs
Inside the nfs mount, create back and front directories for ecryptfs
Use ecryptfs to mount one directory onto the other
Create a few files inside the ecryptfs mount
Set ftrace to monitor ecryptfs_do_unlink() and ecryptfs_evict_inode()
Delete files
Unmount ecryptfs
ftrace output before patch:
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
1) | /* Tracing enabled */
1) | /* Writing to files */
1) | /* Unlinking first file */
0) * 27804.34 us | ecryptfs_do_unlink();
1) | /* rm'ing second file */
------------------------------------------
0) unlink-1384 => rm-1385
------------------------------------------
0) * 32828.63 us | ecryptfs_do_unlink();
1) | /* Unmounting eCryptFS */
------------------------------------------
1) trace-a-1365 => umount-1387
------------------------------------------
1) 2.408 us | ecryptfs_evict_inode();
1) 8.914 us | ecryptfs_evict_inode();
1) 3.344 us | ecryptfs_evict_inode();
ftrace output after patch:
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
0) | /* Tracing enabled */
0) | /* Writing to files */
0) | /* Unlinking first file */
1) * 24728.81 us | ecryptfs_do_unlink();
1) + 20.923 us | ecryptfs_evict_inode();
0) | /* rm'ing second file */
------------------------------------------
1) unlink-1333 => rm-1334
------------------------------------------
1) * 41093.71 us | ecryptfs_do_unlink();
1) + 11.384 us | ecryptfs_evict_inode();
0) | /* Unmounting eCryptFS */
------------------------------------------
0) trace-a-1314 => umount-1336
------------------------------------------
0) 2.986 us | ecryptfs_evict_inode();
On Fri, May 08, 2020 at 09:31:33AM -0500, Jeffrey Mitchell wrote:
> On asynchronous base filesystems like NFS, eCryptFS leaves inodes for
> deleted files in the cache until unmounting. Change call in
> ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably
> evict inodes from the cache even on top of NFS.
>
> Signed-off-by: Dan Robertson <daniel.robertson@starlab.io>
> Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
> ---
> fs/ecryptfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index e23752d..f7594b6 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
> goto out_unlock;
> }
> fsstack_copy_attr_times(dir, lower_dir_inode);
> - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
> + drop_nlink(inode);
> inode->i_ctime = dir->i_ctime;
> out_unlock:
> dput(lower_dentry);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ecryptfs: Fix inodes not being evicted until unmount
@ 2020-12-18 19:07 Jeffrey Mitchell
2020-12-18 19:10 ` Jeffrey Mitchell
2021-01-30 17:06 ` Tyler Hicks
0 siblings, 2 replies; 6+ messages in thread
From: Jeffrey Mitchell @ 2020-12-18 19:07 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, Dan Robertson, Jeffrey Mitchell
On asynchronous base filesystems like NFS, eCryptFS leaves inodes for
deleted files in the cache until unmounting. Change call in
ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably
evict inodes from the cache even on top of NFS.
Signed-off-by: Dan Robertson <daniel.robertson@starlab.io>
Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
---
fs/ecryptfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index e23752d..f7594b6 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
goto out_unlock;
}
fsstack_copy_attr_times(dir, lower_dir_inode);
- set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
+ drop_nlink(inode);
inode->i_ctime = dir->i_ctime;
out_unlock:
dput(lower_dentry);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
2020-12-18 19:07 Jeffrey Mitchell
@ 2020-12-18 19:10 ` Jeffrey Mitchell
2021-01-30 17:06 ` Tyler Hicks
1 sibling, 0 replies; 6+ messages in thread
From: Jeffrey Mitchell @ 2020-12-18 19:10 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, Dan Robertson, Jeffrey Mitchell
Steps to reproduce issue:
Mount nfs
Inside the nfs mount, create back and front directories for ecryptfs
Use ecryptfs to mount one directory onto the other
Create a few files inside the ecryptfs mount
Set ftrace to monitor ecryptfs_do_unlink() and ecryptfs_evict_inode()
Delete files
Unmount ecryptfs
ftrace output before patch:
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
1) | /* Tracing enabled */
1) | /* Writing to files */
1) | /* Unlinking first file */
0) * 27804.34 us | ecryptfs_do_unlink();
1) | /* rm'ing second file */
------------------------------------------
0) unlink-1384 => rm-1385
------------------------------------------
0) * 32828.63 us | ecryptfs_do_unlink();
1) | /* Unmounting eCryptFS */
------------------------------------------
1) trace-a-1365 => umount-1387
------------------------------------------
1) 2.408 us | ecryptfs_evict_inode();
1) 8.914 us | ecryptfs_evict_inode();
1) 3.344 us | ecryptfs_evict_inode();
ftrace output after patch:
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
0) | /* Tracing enabled */
0) | /* Writing to files */
0) | /* Unlinking first file */
1) * 24728.81 us | ecryptfs_do_unlink();
1) + 20.923 us | ecryptfs_evict_inode();
0) | /* rm'ing second file */
------------------------------------------
1) unlink-1333 => rm-1334
------------------------------------------
1) * 41093.71 us | ecryptfs_do_unlink();
1) + 11.384 us | ecryptfs_evict_inode();
0) | /* Unmounting eCryptFS */
------------------------------------------
0) trace-a-1314 => umount-1336
------------------------------------------
0) 2.986 us | ecryptfs_evict_inode();
On Fri, Dec 18, 2020 at 01:07:30PM -0600, Jeffrey Mitchell wrote:
> On asynchronous base filesystems like NFS, eCryptFS leaves inodes for
> deleted files in the cache until unmounting. Change call in
> ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably
> evict inodes from the cache even on top of NFS.
>
> Signed-off-by: Dan Robertson <daniel.robertson@starlab.io>
> Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
> ---
> fs/ecryptfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index e23752d..f7594b6 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
> goto out_unlock;
> }
> fsstack_copy_attr_times(dir, lower_dir_inode);
> - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
> + drop_nlink(inode);
> inode->i_ctime = dir->i_ctime;
> out_unlock:
> dput(lower_dentry);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
2020-12-18 19:07 Jeffrey Mitchell
2020-12-18 19:10 ` Jeffrey Mitchell
@ 2021-01-30 17:06 ` Tyler Hicks
2021-02-02 15:22 ` Jeffrey Mitchell
1 sibling, 1 reply; 6+ messages in thread
From: Tyler Hicks @ 2021-01-30 17:06 UTC (permalink / raw)
To: Jeffrey Mitchell; +Cc: ecryptfs, linux-kernel, Dan Robertson
On 2020-12-18 13:07:30, Jeffrey Mitchell wrote:
> On asynchronous base filesystems like NFS, eCryptFS leaves inodes for
> deleted files in the cache until unmounting. Change call in
> ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably
> evict inodes from the cache even on top of NFS.
>
> Signed-off-by: Dan Robertson <daniel.robertson@starlab.io>
> Signed-off-by: Jeffrey Mitchell <jeffrey.mitchell@starlab.io>
Hey Jeffrey and Dan - thanks for the patch! Unfortunately, I think this
would allow the eCryptfs inode's nlink count to get out of sync with the
lower inode's nlink count in the case of direct manipulation to the
lower filesystem.
Is the condition that you're trying to fix a result of going through the
this code path?
ecryptfs_unlink() -> ecryptfs_do_unlink() -> vfs_unlink() -> nfs_unlink() -> nfs_sillyrename() -> nfs_async_unlink()
Tyler
> ---
> fs/ecryptfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index e23752d..f7594b6 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
> goto out_unlock;
> }
> fsstack_copy_attr_times(dir, lower_dir_inode);
> - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
> + drop_nlink(inode);
> inode->i_ctime = dir->i_ctime;
> out_unlock:
> dput(lower_dentry);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
2021-01-30 17:06 ` Tyler Hicks
@ 2021-02-02 15:22 ` Jeffrey Mitchell
0 siblings, 0 replies; 6+ messages in thread
From: Jeffrey Mitchell @ 2021-02-02 15:22 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs, linux-kernel, Dan Robertson
On Sat, Jan 30, 2021 at 11:06:40AM -0600, Tyler Hicks wrote:
> Hey Jeffrey and Dan - thanks for the patch! Unfortunately, I think this
> would allow the eCryptfs inode's nlink count to get out of sync with the
> lower inode's nlink count in the case of direct manipulation to the
> lower filesystem.
Hmm. What if I instead synchronize it before calling vfs_unlink(), then
call drop_nlink() if vfs_unlink() succeeds?
> Is the condition that you're trying to fix a result of going through the
> this code path?
>
> ecryptfs_unlink() -> ecryptfs_do_unlink() -> vfs_unlink() -> nfs_unlink() -> nfs_sillyrename() -> nfs_async_unlink()
Yes, that is the code path that causes it.
V/R,
Jeffrey Mitchell
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-02 15:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 14:31 [PATCH] ecryptfs: Fix inodes not being evicted until unmount Jeffrey Mitchell
2020-05-08 14:32 ` Jeffrey Mitchell
2020-12-18 19:07 Jeffrey Mitchell
2020-12-18 19:10 ` Jeffrey Mitchell
2021-01-30 17:06 ` Tyler Hicks
2021-02-02 15:22 ` Jeffrey Mitchell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).