From: Dave Chinner <david@fromorbit.com>
To: "Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>
Cc: linux-xfs@vger.kernel.org,
"Darrick J. Wong" <darrick.wong@oracle.com>,
Zorro Lang <zlang@redhat.com>
Subject: Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
Date: Wed, 16 Aug 2017 23:35:02 +1000 [thread overview]
Message-ID: <20170816133502.GN21024@dastard> (raw)
In-Reply-To: <20170816071649.GA4143@debian.home>
On Wed, Aug 16, 2017 at 04:16:53AM -0300, Ernesto A. Fernández wrote:
> On Wed, Aug 16, 2017 at 10:25:11AM +1000, Dave Chinner wrote:
> > On Tue, Aug 15, 2017 at 04:29:39PM -0300, Ernesto A. Fernández wrote:
> > > On Tue, Aug 15, 2017 at 06:44:30PM +1000, Dave Chinner wrote:
> > > > On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
> > > > But I have to ask - why do we even need to modify the mode first?
> > > > Why not change the ACL first, then modify the mode+timestamp? If
> > > > setting the ACL fails, then we don't have anything to undo and all
> > > > is good....
> > >
> > > I intended for the mode to be committed as part of the same transaction
> > > that sets or removes the ACL. In my mind making the changes later, as part
> > > of a separate transaction, would have meant that a crash between the two
> > > left the filesystem in an inconsistent state,
> >
> > No, it will not leave the fileystem in an inconsistent state. It
> > will leave the inode permissions in an /unwanted/ state, but there
> > is no filesystem metadata inconsistency.
> >
> > > with a new ACL but without
> > > the corresponding mode bits.
> >
> > Yup, but that's no different from right now, where a crash after
> > setting the mode bits could be applied but the ACL update is
> > missing.
> >
> > Either way is even rarely than "crash at the wrong time" implies,
> > because we've also got to have a complete journal checkpoint occur
> > between the two operations and then crash between the checkpoint and
> > the second operation. Yes, it's possible, but in the entire time
> > I've been working on XFS (almost 15 years now) I can count on one
> > hand the number of times such a problem has occurred and been
> > reported...
> >
> > So, it's a rare problem, and one that will get solved in time
> > because there's much more to solving the problem than just this
> > case. e.g. I worte this in 2008:
> >
> > http://xfs.org/index.php/Improving_Metadata_Performance_By_Reducing_Journal_Overhead#Atomic_Multi-Transaction_Operations
> >
> > And we've really only got the infrastructure we could use to
> > implement this in a widespread manner with the rmap/reflink
> > functionality. But implementing it will require a large amount of
> > re-organisation of filesystem operations, so it's something that
> > will take time to roll out.
>
> Alright, thanks for the explanation.
>
> > With that in mind, here's waht I suggested above: set the mode after
> > the xattr. I haven't tested it - can you check it solves the problem
> > case you are testing?
>
> It does. Of course the test still fails, as I said before, now claiming that
> the filesystem is inconsistent. But that's a separate issue.
It shouldn't be - what's the error that is being reported?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2017-08-16 13:35 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-14 8:18 [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails Ernesto A. Fernández
2017-08-14 22:28 ` Darrick J. Wong
2017-08-15 0:58 ` Ernesto A. Fernández
2017-08-15 2:00 ` Dave Chinner
2017-08-15 6:18 ` Ernesto A. Fernández
2017-08-15 8:44 ` Dave Chinner
2017-08-15 19:29 ` Ernesto A. Fernández
2017-08-16 0:25 ` Dave Chinner
2017-08-16 7:16 ` Ernesto A. Fernández
2017-08-16 13:35 ` Dave Chinner [this message]
2017-08-16 19:31 ` Ernesto A. Fernández
2017-08-17 0:00 ` Dave Chinner
2017-08-17 5:34 ` Ernesto A. Fernández
2017-08-17 6:34 ` Dave Chinner
2017-12-07 17:31 ` Bill O'Donnell
2017-12-07 17:38 ` Bill O'Donnell
2017-12-07 17:43 ` Darrick J. Wong
2017-12-07 17:51 ` Bill O'Donnell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170816133502.GN21024@dastard \
--to=david@fromorbit.com \
--cc=darrick.wong@oracle.com \
--cc=ernesto.mnd.fernandez@gmail.com \
--cc=linux-xfs@vger.kernel.org \
--cc=zlang@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).