From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 10072C4361B for ; Tue, 15 Dec 2020 13:17:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CC24122225 for ; Tue, 15 Dec 2020 13:17:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728333AbgLONRB (ORCPT ); Tue, 15 Dec 2020 08:17:01 -0500 Received: from mail.kernel.org ([198.145.29.99]:50164 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727621AbgLONQz (ORCPT ); Tue, 15 Dec 2020 08:16:55 -0500 Message-ID: <73ed2ee27cb21b5879d030f5478839507dc35efd.camel@kernel.org> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1608038174; bh=lD3zVNl/UdnKgognn1VdyUcVGB4m6JnB/aKuzonKIfc=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=qTxErsFSHmx/BkWTpp1vtVNY0wohqAP911Vfi+YyEl7TJIASQsPTzXJzpx0FEF5Nc JcQGOvT5l4J7+Aou6785fCmFqBMiikiRjo1MXCwAe+5wuKlv8IuUn2bMWD0dX4oRLa CnhiWTiJZioz7JwOycAbaJxjd14fTNdqEGoSwxQsqBY+vhPNa9/laaag4xG0+F5sy2 AhWAlppjclNcmF3nwRtGGLL5SCjpUmj5Mq0CCA7RShgAmmiUeFuXtLYezfZQzTdrx0 BG8PeZqANwN6xbplobyEWWQVcBHEa/uhMGZSWu+6A36bQkVpbyQHWkuPTqOASCOnGo 8Vw9jiExYUTeQ== Subject: Re: [RFC PATCH 2/2] overlayfs: propagate errors from upper to overlay sb in sync_fs From: Jeff Layton To: Vivek Goyal Cc: Amir Goldstein , Sargun Dhillon , Miklos Szeredi , overlayfs , Linux FS-devel Mailing List , Matthew Wilcox , NeilBrown , Jan Kara Date: Tue, 15 Dec 2020 08:16:12 -0500 In-Reply-To: <979d78d04d882744d944f5723ad7a98b14badf8b.camel@kernel.org> References: <20201213132713.66864-1-jlayton@kernel.org> <20201213132713.66864-3-jlayton@kernel.org> <20201214213843.GA3453@redhat.com> <979d78d04d882744d944f5723ad7a98b14badf8b.camel@kernel.org> Content-Type: text/plain; charset="ISO-8859-15" User-Agent: Evolution 3.38.2 (3.38.2-1.fc33) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-unionfs@vger.kernel.org On Mon, 2020-12-14 at 18:53 -0500, Jeff Layton wrote: > On Mon, 2020-12-14 at 16:38 -0500, Vivek Goyal wrote: > > On Sun, Dec 13, 2020 at 08:27:13AM -0500, Jeff Layton wrote: > > > Peek at the upper layer's errseq_t at mount time for volatile mounts, > > > and record it in the per-sb info. In sync_fs, check for an error since > > > the recorded point and set it in the overlayfs superblock if there was > > > one. > > > > > > Signed-off-by: Jeff Layton > > > --- > > > > While we are solving problem for non-volatile overlay mount, I also > > started thinking, what about non-volatile overlay syncfs() writeback errors. > > Looks like these will not be reported to user space at all as of now > > (because we never update overlay_sb->s_wb_err ever). > > > > A patch like this might fix it. (compile tested only). > > > > overlayfs: Report syncfs() errors to user space > > > > Currently, syncfs(), calls filesystem ->sync_fs() method but ignores the > > return code. But certain writeback errors can still be reported on > > syncfs() by checking errors on super block. > > > > ret2 = errseq_check_and_advance(&sb->s_wb_err, &f.file->f_sb_err); > > > > For the case of overlayfs, we never set overlayfs super block s_wb_err. That > > means sync() will never report writeback errors on overlayfs uppon syncfs(). > > > > Fix this by updating overlay sb->sb_wb_err upon ->sync_fs() call. And that > > should mean that user space syncfs() call should see writeback errors. > > > > ovl_fsync() does not need anything special because if there are writeback > > errors underlying filesystem will report it through vfs_fsync_range() return > > code and user space will see it. > > > > Signed-off-by: Vivek Goyal > > --- > >  fs/overlayfs/ovl_entry.h | 1 + > >  fs/overlayfs/super.c | 14 +++++++++++--- > >  2 files changed, 12 insertions(+), 3 deletions(-) > > > > Index: redhat-linux/fs/overlayfs/super.c > > =================================================================== > > --- redhat-linux.orig/fs/overlayfs/super.c 2020-12-14 15:33:43.934400880 -0500 > > +++ redhat-linux/fs/overlayfs/super.c 2020-12-14 16:15:07.127400880 -0500 > > @@ -259,7 +259,7 @@ static int ovl_sync_fs(struct super_bloc > >  { > >   struct ovl_fs *ofs = sb->s_fs_info; > >   struct super_block *upper_sb; > > - int ret; > > + int ret, ret2; > >   > > > > > > > >   if (!ovl_upper_mnt(ofs)) > >   return 0; > > @@ -283,7 +283,14 @@ static int ovl_sync_fs(struct super_bloc > >   ret = sync_filesystem(upper_sb); > >   up_read(&upper_sb->s_umount); > >   > > > > > > > > - return ret; > > + if (errseq_check(&upper_sb->s_wb_err, sb->s_wb_err)) { > > + /* Upper sb has errors since last time */ > > + spin_lock(&ofs->errseq_lock); > > + ret2 = errseq_check_and_advance(&upper_sb->s_wb_err, > > + &sb->s_wb_err); > > + spin_unlock(&ofs->errseq_lock); > > + } > > + return ret ? ret : ret2; > > I think this is probably not quite right. > > The problem I think is that the SEEN flag is always going to end up > being set in sb->s_wb_err, and that is going to violate the desired > semantics. If the writeback error occurred after all fd's were closed, > then the next opener wouldn't see it and you'd lose the error. > > We probably need a function to cleanly propagate the error from one > errseq_t to another so that that doesn't occur. I'll have to think about > it. > So, the problem is that we can't guarantee that we'll have an open file when sync_fs is called. So if you do the check_and_advance in the context of a sync() syscall, you'll effectively ensure that a later opener on the upper layer won't see the error (since the upper_sb's errseq_t will be marked SEEN. It's not clear to me what semantics you want in the following situation: mount upper layer mount overlayfs with non-volatile upper layer do "stuff" on overlayfs, and close all files on overlayfs get a writeback error on upper layer call sync() (sync_fs gets run) open file on upper layer mount call syncfs() on upper-layer fd Should that last syncfs error report an error? Also, suppose if at the end we instead opened a file on overlayfs and issued the syncfs() there -- should we see the error in that case? > >  } > >   > > > > > > > >  /** > > @@ -1873,6 +1880,7 @@ static int ovl_fill_super(struct super_b > >   if (!cred) > >   goto out_err; > >   > > > > > > > > + spin_lock_init(&ofs->errseq_lock); > >   /* Is there a reason anyone would want not to share whiteouts? */ > >   ofs->share_whiteout = true; > >   > > > > > > > > @@ -1945,7 +1953,7 @@ static int ovl_fill_super(struct super_b > >   > > > > > > > >   sb->s_stack_depth = ovl_upper_mnt(ofs)->mnt_sb->s_stack_depth; > >   sb->s_time_gran = ovl_upper_mnt(ofs)->mnt_sb->s_time_gran; > > - > > + sb->s_wb_err = errseq_sample(&ovl_upper_mnt(ofs)->mnt_sb->s_wb_err); > >   } > >   oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers); > >   err = PTR_ERR(oe); > > Index: redhat-linux/fs/overlayfs/ovl_entry.h > > =================================================================== > > --- redhat-linux.orig/fs/overlayfs/ovl_entry.h 2020-12-14 15:33:43.934400880 -0500 > > +++ redhat-linux/fs/overlayfs/ovl_entry.h 2020-12-14 15:34:13.509400880 -0500 > > @@ -79,6 +79,7 @@ struct ovl_fs { > >   atomic_long_t last_ino; > >   /* Whiteout dentry cache */ > >   struct dentry *whiteout; > > + spinlock_t errseq_lock; > >  }; > >   > > > > > > > >  static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs) > > > -- Jeff Layton