Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp2153193pxu; Fri, 18 Dec 2020 06:47:10 -0800 (PST) X-Google-Smtp-Source: ABdhPJwUJs8DTphjCNyme0axOQGN6qk6UKzd8EHuA3oXudwyZAf4x6AI/NgYErZy6qPgYVw3hU+G X-Received: by 2002:a17:906:589:: with SMTP id 9mr4369710ejn.229.1608302830288; Fri, 18 Dec 2020 06:47:10 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1608302830; cv=none; d=google.com; s=arc-20160816; b=RH8ONavNgp0o2dO+fa3SxYKtsyY2QAhEPQlbexkpI060YO+Ck/Wl7W8IC8bKOOqRxU FvOM4oPo8e93RZIZT+PxL/E2SmbFxP9h25EdGUl5lsxXEzvDOnsfeur7DE78AEeysgS5 PPgr19TjsbFEGV1V76gHAYvEvVLGZJYVAJ2SOFaZDAE2lhfjxS17ABGYAGRP2RmrZCTh uevr2pcLkPOAEvYEReYDFwJRVwFo6UjF/mWUyISsucKCpSyqbnlpdMDfGHHYRBBzmh6Q tyW7tVf/IYYsknbvps4jHpAuMD4Xv3ebh+uL4fWoVqyJuS519i8Su7kcef5nfoPijCSV jv+A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=CthNdoUY/FIFXJCBduT4k2Ir0PRGPQTHNOp+0Jn+7JM=; b=ZbWV2Bv/xXJVsfH0dhFPb9O8Na86ZfCd42WtbCBcDoaU6TwI7ascG7KGH1HNQtxaGZ s2fUejFruhBjkhRzahKn+WH/JDbV20dfgID/+R0F0ewsga/uljGRiXLZL3yXypD0Ui+b OwM7K7p3DXZ514tsSff/zw2Qo2IhjUmHeERL/mk4Y6xHcRxctvcOZc6cAtN5zjV7aDrt MwKOQh/GA4aezxuLUE04187/42OO9MOAerJxgks3V9m6qs6nvrfHef5qxM3BQag7Op09 QImocEs0f8mOE2uDeEZEB7m+0iAGDa3bmgGNz4TetbyO7Q5L9tSttJG433d8aKzqsLpF 3GZg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=P7kYKd1E; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id a13si6424339edb.419.2020.12.18.06.46.46; Fri, 18 Dec 2020 06:47:10 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=P7kYKd1E; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727863AbgLROpx (ORCPT + 99 others); Fri, 18 Dec 2020 09:45:53 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:27015 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726567AbgLROpw (ORCPT ); Fri, 18 Dec 2020 09:45:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1608302665; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=CthNdoUY/FIFXJCBduT4k2Ir0PRGPQTHNOp+0Jn+7JM=; b=P7kYKd1EaBJ2pQeY16aE4BNqPSLUlEGyuUH/J17wjWubKzAZkt8JNlrtmJf+npqgw/C6jY 91PFQbCZkKeq/G87AHO65ON3RZMggW6t+c/nmo8GqE+ImB2fX4IJg3d/hmUq5nIaEb/eg9 8L2aXxHSG5+9wnjhKIo8yVNlnR6Xy+o= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-169-pEZB78OdMT2Hg7zsV1RQcA-1; Fri, 18 Dec 2020 09:44:21 -0500 X-MC-Unique: pEZB78OdMT2Hg7zsV1RQcA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 43D98800688; Fri, 18 Dec 2020 14:44:19 +0000 (UTC) Received: from horse.redhat.com (ovpn-115-223.rdu2.redhat.com [10.10.115.223]) by smtp.corp.redhat.com (Postfix) with ESMTP id B815B60CED; Fri, 18 Dec 2020 14:44:18 +0000 (UTC) Received: by horse.redhat.com (Postfix, from userid 10451) id 3E856220BCF; Fri, 18 Dec 2020 09:44:18 -0500 (EST) Date: Fri, 18 Dec 2020 09:44:18 -0500 From: Vivek Goyal To: Jeffrey Layton Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-unionfs@vger.kernel.org, jlayton@kernel.org, amir73il@gmail.com, sargun@sargun.me, miklos@szeredi.hu, willy@infradead.org, jack@suse.cz, neilb@suse.com, viro@zeniv.linux.org.uk Subject: Re: [PATCH 3/3] overlayfs: Check writeback errors w.r.t upper in ->syncfs() Message-ID: <20201218144418.GA3424@redhat.com> References: <20201216233149.39025-1-vgoyal@redhat.com> <20201216233149.39025-4-vgoyal@redhat.com> <20201217200856.GA707519@tleilax.poochiereds.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201217200856.GA707519@tleilax.poochiereds.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 17, 2020 at 03:08:56PM -0500, Jeffrey Layton wrote: > On Wed, Dec 16, 2020 at 06:31:49PM -0500, Vivek Goyal wrote: > > Check for writeback error on overlay super block w.r.t "struct file" > > passed in ->syncfs(). > > > > As of now real error happens on upper sb. So this patch first propagates > > error from upper sb to overlay sb and then checks error w.r.t struct > > file passed in. > > > > Jeff, I know you prefer that I should rather file upper file and check > > error directly on on upper sb w.r.t this real upper file. While I was > > implementing that I thought what if file is on lower (and has not been > > copied up yet). In that case shall we not check writeback errors and > > return back to user space? That does not sound right though because, > > we are not checking for writeback errors on this file. Rather we > > are checking for any error on superblock. Upper might have an error > > and we should report it to user even if file in question is a lower > > file. And that's why I fell back to this approach. But I am open to > > change it if there are issues in this method. > > > > Signed-off-by: Vivek Goyal > > --- > > fs/overlayfs/ovl_entry.h | 2 ++ > > fs/overlayfs/super.c | 15 ++++++++++++--- > > 2 files changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h > > index 1b5a2094df8e..a08fd719ee7b 100644 > > --- a/fs/overlayfs/ovl_entry.h > > +++ b/fs/overlayfs/ovl_entry.h > > @@ -79,6 +79,8 @@ struct ovl_fs { > > atomic_long_t last_ino; > > /* Whiteout dentry cache */ > > struct dentry *whiteout; > > + /* Protects multiple sb->s_wb_err update from upper_sb . */ > > + spinlock_t errseq_lock; > > }; > > > > static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs) > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > > index b4d92e6fa5ce..e7bc4492205e 100644 > > --- a/fs/overlayfs/super.c > > +++ b/fs/overlayfs/super.c > > @@ -291,7 +291,7 @@ int ovl_syncfs(struct file *file) > > struct super_block *sb = file->f_path.dentry->d_sb; > > struct ovl_fs *ofs = sb->s_fs_info; > > struct super_block *upper_sb; > > - int ret; > > + int ret, ret2; > > > > ret = 0; > > down_read(&sb->s_umount); > > @@ -310,10 +310,18 @@ int ovl_syncfs(struct file *file) > > ret = sync_filesystem(upper_sb); > > up_read(&upper_sb->s_umount); > > > > + /* Update overlay sb->s_wb_err */ > > + if (errseq_check(&upper_sb->s_wb_err, sb->s_wb_err)) { > > + /* Upper sb has errors since last time */ > > + spin_lock(&ofs->errseq_lock); > > + errseq_check_and_advance(&upper_sb->s_wb_err, &sb->s_wb_err); > > + spin_unlock(&ofs->errseq_lock); > > + } > > So, the problem here is that the resulting value in sb->s_wb_err is > going to end up with the REPORTED flag set (using the naming in my > latest set). So, a later opener of a file on sb->s_wb_err won't see it. > > For instance, suppose you call sync() on the box and does the above > check and advance. Then, you open the file and call syncfs() and get > back no error because REPORTED flag was set when you opened. That error > will then be lost. Hi Jeff, In this patch, I am doing this only in ->syncfs() path and not in ->sync_fs() path. IOW, errseq_check_and_advance() will take place only if there is a valid "struct file" passed in. That means there is a consumer of the error and that means it should be fine to set the sb->s_wb_err as SEEN/REPORTED, right? If we end up plumbming "struct file" in existing ->sync_fs() routine, then I will call this only if a non NULL struct file has been passed in. Otherwise skip this step. IOW, sync() call will not result in errseq_check_and_advance() instead a syncfs() call will. > > > > > + ret2 = errseq_check_and_advance(&sb->s_wb_err, &file->f_sb_err); > > out: > > up_read(&sb->s_umount); > > - return ret; > > + return ret ? ret : ret2; > > } > > > > /** > > @@ -1903,6 +1911,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > > 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; > > > > @@ -1975,7 +1984,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > > > > 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); > > This will mark the error on the upper_sb as REPORTED, and that's not > really that's the case if you're just using it set s_wb_err in the > overlay. You might want to use errseq_peek in this situation. For now I am still looking at existing code and not new code. Because I belive that new code does not change existing behavior instead provides additional functionality to allow sampling the error without marking it seen as well as provide helper to not force seeing an unseen error. So current errseq_sample() does not mark error SEEN. And if it is an unseen error, we will get 0 and be forced to see the error next time. One small issue with this is that say upper has unseen error. Now we mount overlay and save that value in sb->s_wb_err (unseen). Say a file is opened on upper and error is now seen on upper. But we still have unseen error cached in overlay and if overlay fd is now opened, f->f_sb_err will be 0 and it will be forced to see err on next syncfs(). IOW, despite the fact that overlay fd was opened after upper sb had been marked seen, it still will see error. I think it probably is not a big issue. Vivek > > > } > > oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers); > > err = PTR_ERR(oe); > > -- > > 2.25.4 > > >