Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1447899 > unrolled thread

[RFC PATCH v2 0/3] fix overlayfs locks and leases

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2 0/3] fix overlayfs locks and leases Miklos Szeredi <mszeredi@redhat.com> - 2016-07-21 16:00 +0200
    [RFC PATCH v2 3/3] vfs: do get_write_access() on upper layer of overlayfs Miklos Szeredi <mszeredi@redhat.com> - 2016-07-21 16:00 +0200
    Re: [RFC PATCH v2 0/3] fix overlayfs locks and leases Jeff Layton <jlayton@poochiereds.net> - 2016-07-21 22:20 +0200

#1447899 — [RFC PATCH v2 0/3] fix overlayfs locks and leases

I've split out the writecount handling and changed it around so that
underlying layers are consistent and yet leases work correctly on
overlayfs.

Also pushed to the tip of

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next

Thanks,
Miklos
---

Miklos Szeredi (3):
  locks: fix file locking on overlayfs
  vfs: make argument of d_real_inode() const
  vfs: do get_write_access() on upper layer of overlayfs

 fs/locks.c              | 53 ++++++++++++++++++++++++++++---------------------
 fs/namespace.c          |  2 +-
 fs/open.c               | 17 +++++++++++++---
 fs/overlayfs/super.c    |  2 +-
 include/linux/dcache.h  |  5 +++--
 include/linux/fs.h      | 16 +++++++++++++--
 include/uapi/linux/fs.h |  1 +
 7 files changed, 64 insertions(+), 32 deletions(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1447900 — [RFC PATCH v2 3/3] vfs: do get_write_access() on upper layer of overlayfs

The problem with writecount is: we want consistent handling of it for
underlying filesystems as well as overlayfs.  Making sure i_writecount is
correct on all layers is difficult.  Instead this patch makes sure that
when write access is acquired, it's always done on the underlying writable
layer (called the upper layer).  We must also make sure to look at the
writecount on this layer when checking for conflicting leases.

Open for write already updates the upper layer's writecount.  Leaving only
truncate.

For truncate copy up must happen before get_write_access() so that the
writecount is updated on the upper layer.  Problem with this is if
something fails after that, then copy-up was done needlessly.  E.g. if
break_lease() was interrupted.  Probably not a big deal in practice.

Another interesting case is if there's a denywrite on a lower file that is
then opened for write or truncated.  With this patch these will succeed,
which is somewhat counterintuitive.  But I think it's still acceptable,
considering that the copy-up does actually create a different file, so the
old, denywrite mapping won't be touched.

On non-overlayfs d_real() is an identity function and d_real_inode() is
equivalent to d_inode() so this patch doesn't change behavior in that case.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/locks.c |  3 ++-
 fs/open.c  | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index c1656cff53ee..b242d5b99589 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1618,7 +1618,8 @@ check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
 	if (flags & FL_LAYOUT)
 		return 0;
 
-	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
+	if ((arg == F_RDLCK) &&
+	    (atomic_read(&d_real_inode(dentry)->i_writecount) > 0))
 		return -EAGAIN;
 
 	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
diff --git a/fs/open.c b/fs/open.c
index 451fed14a843..76d8d97b5136 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -68,6 +68,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 long vfs_truncate(const struct path *path, loff_t length)
 {
 	struct inode *inode;
+	struct dentry *upperdentry;
 	long error;
 
 	inode = path->dentry->d_inode;
@@ -90,7 +91,17 @@ long vfs_truncate(const struct path *path, loff_t length)
 	if (IS_APPEND(inode))
 		goto mnt_drop_write_and_out;
 
-	error = get_write_access(inode);
+	/*
+	 * If this is an overlayfs then do as if opening the file so we get
+	 * write access on the upper inode, not on the overlay inode.  For
+	 * non-overlay filesystems d_real() is an identity function.
+	 */
+	upperdentry = d_real(path->dentry, NULL, O_WRONLY);
+	error = PTR_ERR(upperdentry);
+	if (IS_ERR(upperdentry))
+		goto mnt_drop_write_and_out;
+
+	error = get_write_access(upperdentry->d_inode);
 	if (error)
 		goto mnt_drop_write_and_out;
 
@@ -109,7 +120,7 @@ long vfs_truncate(const struct path *path, loff_t length)
 		error = do_truncate(path->dentry, length, 0, NULL);
 
 put_write_and_out:
-	put_write_access(inode);
+	put_write_access(upperdentry->d_inode);
 mnt_drop_write_and_out:
 	mnt_drop_write(path->mnt);
 out:
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1448115

On Thu, 2016-07-21 at 15:53 +0200, Miklos Szeredi wrote:
> I've split out the writecount handling and changed it around so that
> underlying layers are consistent and yet leases work correctly on
> overlayfs.
> 
> Also pushed to the tip of
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
> 
> Thanks,
> Miklos
> ---
> 
> Miklos Szeredi (3):
>   locks: fix file locking on overlayfs
>   vfs: make argument of d_real_inode() const
>   vfs: do get_write_access() on upper layer of overlayfs
> 
>  fs/locks.c              | 53 ++++++++++++++++++++++++++++---------------------
>  fs/namespace.c          |  2 +-
>  fs/open.c               | 17 +++++++++++++---
>  fs/overlayfs/super.c    |  2 +-
>  include/linux/dcache.h  |  5 +++--
>  include/linux/fs.h      | 16 +++++++++++++--
>  include/uapi/linux/fs.h |  1 +
>  7 files changed, 64 insertions(+), 32 deletions(-)
> 

Looks pretty sane overall.

Also, when I mentioned accessing the writable layer in openwrt in the
last set, I forgot that you typically only do reads on it, so the
writecount wouldn't be affected in the case of accessing to do backups.
So, I'm not sure I had a legit objection to the earlier patch, but I
think this looks a little cleaner anyway:

Acked-by: Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web