From: Dan Williams <dan.j.williams@intel.com>
To: Jane Chu <jane.chu@oracle.com>
Cc: Dave Chinner <david@fromorbit.com>,
"Darrick J. Wong" <djwong@kernel.org>,
Christoph Hellwig <hch@infradead.org>,
linux-xfs <linux-xfs@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH 3/5] vfs: add a zero-initialization mode to fallocate
Date: Mon, 27 Sep 2021 17:08:03 -0700 [thread overview]
Message-ID: <CAPcyv4gys1F6G1cgXk2UOcr27GNBAfc+ZBoh7MAwFVu5cqfXDg@mail.gmail.com> (raw)
In-Reply-To: <b0861cd0-f5c3-6a56-29f9-cd4421c221c4@oracle.com>
On Mon, Sep 27, 2021 at 2:58 PM Jane Chu <jane.chu@oracle.com> wrote:
>
> On 9/27/2021 2:07 PM, Dave Chinner wrote:
> > On Thu, Sep 23, 2021 at 06:35:16PM -0700, Darrick J. Wong wrote:
> >> On Thu, Sep 23, 2021 at 06:21:19PM -0700, Jane Chu wrote:
> >>>
> >>> On 9/23/2021 6:18 PM, Dan Williams wrote:
> >>>> On Thu, Sep 23, 2021 at 3:54 PM Dave Chinner <david@fromorbit.com> wrote:
> >>>>>
> >>>>> On Wed, Sep 22, 2021 at 10:42:11PM -0700, Dan Williams wrote:
> >>>>>> On Wed, Sep 22, 2021 at 7:43 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >>>>>>>
> >>>>>>> On Wed, Sep 22, 2021 at 6:42 PM Dave Chinner <david@fromorbit.com> wrote:
> >>>>>>> [..]
> >>>>>>>> Hence this discussion leads me to conclude that fallocate() simply
> >>>>>>>> isn't the right interface to clear storage hardware poison state and
> >>>>>>>> it's much simpler for everyone - kernel and userspace - to provide a
> >>>>>>>> pwritev2(RWF_CLEAR_HWERROR) flag to directly instruct the IO path to
> >>>>>>>> clear hardware error state before issuing this user write to the
> >>>>>>>> hardware.
> >>>>>>>
> >>>>>>> That flag would slot in nicely in dax_iomap_iter() as the gate for
> >>>>>>> whether dax_direct_access() should allow mapping over error ranges,
> >>>>>>> and then as a flag to dax_copy_from_iter() to indicate that it should
> >>>>>>> compare the incoming write to known poison and clear it before
> >>>>>>> proceeding.
> >>>>>>>
> >>>>>>> I like the distinction, because there's a chance the application did
> >>>>>>> not know that the page had experienced data loss and might want the
> >>>>>>> error behavior. The other service the driver could offer with this
> >>>>>>> flag is to do a precise check of the incoming write to make sure it
> >>>>>>> overlaps known poison and then repair the entire page. Repairing whole
> >>>>>>> pages makes for a cleaner implementation of the code that tries to
> >>>>>>> keep poison out of the CPU speculation path, {set,clear}_mce_nospec().
> >>>>>>
> >>>>>> This flag could also be useful for preadv2() as there is currently no
> >>>>>> way to read the good data in a PMEM page with poison via DAX. So the
> >>>>>> flag would tell dax_direct_access() to again proceed in the face of
> >>>>>> errors, but then the driver's dax_copy_to_iter() operation could
> >>>>>> either read up to the precise byte offset of the error in the page, or
> >>>>>> autoreplace error data with zero's to try to maximize data recovery.
> >>>>>
> >>>>> Yes, it could. I like the idea - say RWF_IGNORE_HWERROR - to read
> >>>>> everything that can be read from the bad range because it's the
> >>>>> other half of the problem RWF_RESET_HWERROR is trying to address.
> >>>>> That is, the operation we want to perform on a range with an error
> >>>>> state is -data recovery-, not "reinitialisation". Data recovery
> >>>>> requires two steps:
> >>>>>
> >>>>> - "try to recover the data from the bad storage"; and
> >>>>> - "reinitialise the data and clear the error state"
> >>>>>
> >>>>> These naturally map to read() and write() operations, not
> >>>>> fallocate(). With RWF flags they become explicit data recovery
> >>>>> operations, unlike fallocate() which needs to imply that "writing
> >>>>> zeroes" == "reset hardware error state". While that reset method
> >>>>> may be true for a specific pmem hardware implementation it is not a
> >>>>> requirement for all storage hardware. It's most definitely not a
> >>>>> requirement for future storage hardware, either.
> >>>>>
> >>>>> It also means that applications have no choice in what data they can
> >>>>> use to reinitialise the damaged range with because fallocate() only
> >>>>> supports writing zeroes. If we've recovered data via a read() as you
> >>>>> suggest we could, then we can rebuild the data from other redundant
> >>>>> information and immediately write that back to the storage, hence
> >>>>> repairing the fault.
> >>>>>
> >>>>> That, in turn, allows the filesystem to turn the RWF_RESET_HWERROR
> >>>>> write into an exclusive operation and hence allow the
> >>>>> reinitialisation with the recovered/repaired state to run atomically
> >>>>> w.r.t. all other filesystem operations. i.e. the reset write
> >>>>> completes the recovery operation instead of requiring separate
> >>>>> "reset" and "write recovered data into zeroed range" steps that
> >>>>> cannot be executed atomically by userspace...
> >>>>
> >>>> /me nods
> >>>>
> >>>> Jane, want to take a run at patches for this ^^^?
> >>>>
> >>>
> >>> Sure, I'll give it a try.
> >>>
> >>> Thank you all for the discussions!
> >>
> >> Cool, thank you!
> >
> > I'd like to propose a slight modification to the API: a single RWF
> > flag called RWF_RECOVER_DATA. On read, this means the storage tries
> > to read all the data it can from the range, and for the parts it
> > can't read data from (cachelines, sectors, whatever) it returns as
> > zeroes.
> >
> > On write, this means the errors over the range get cleared and the
> > user data provided gets written over the top of whatever was there.
> > Filesystems should perform this as an exclusive operation to that
> > range of the file.
> >
> > That way we only need one IOCB_RECOVERY flag, and for communicating
> > with lower storage layers (e.g. dm/md raid and/or hardware) only one
> > REQ_RECOVERY flag is needed in the bio.
> >
> > Thoughts?
>
> Sounds cleaner. Dan, your thoughts?
I like it. I was struggling with a way to unify the flag names, and
"recovery" is a good term for not surprising the caller with zeros.
I.e. don't use this flow to avoid errors, use this flow to maximize
data recovery.
next prev parent reply other threads:[~2021-09-28 0:08 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-18 1:30 [PATCHSET RFC v2 jane 0/5] vfs: enable userspace to reset damaged file storage Darrick J. Wong
2021-09-18 1:30 ` [PATCH 1/5] dax: prepare pmem for use by zero-initializing contents and clearing poisons Darrick J. Wong
2021-09-18 16:54 ` riteshh
2021-09-20 17:22 ` Darrick J. Wong
2021-09-21 4:07 ` riteshh
2021-09-22 18:26 ` Darrick J. Wong
2021-09-22 19:47 ` riteshh
2021-09-22 20:26 ` Dan Williams
2021-09-21 8:34 ` Christoph Hellwig
2021-09-22 18:10 ` Darrick J. Wong
2021-09-18 1:30 ` [PATCH 2/5] iomap: use accelerated zeroing on a block device to zero a file range Darrick J. Wong
2021-09-18 16:55 ` riteshh
2021-09-21 8:29 ` Christoph Hellwig
2021-09-22 18:53 ` Darrick J. Wong
2021-09-21 22:33 ` Dave Chinner
2021-09-22 18:54 ` Darrick J. Wong
2021-09-18 1:31 ` [PATCH 3/5] vfs: add a zero-initialization mode to fallocate Darrick J. Wong
2021-09-18 16:58 ` riteshh
2021-09-20 17:52 ` Eric Biggers
2021-09-20 18:06 ` Darrick J. Wong
2021-09-21 0:44 ` Dave Chinner
2021-09-21 8:31 ` Christoph Hellwig
2021-09-22 2:16 ` Dan Williams
2021-09-22 2:38 ` Darrick J. Wong
2021-09-22 3:59 ` Dave Chinner
2021-09-22 4:13 ` Darrick J. Wong
2021-09-22 5:49 ` Dave Chinner
2021-09-22 21:27 ` Darrick J. Wong
2021-09-23 0:02 ` Darrick J. Wong
2021-09-23 0:44 ` Darrick J. Wong
2021-09-23 1:42 ` Dave Chinner
2021-09-23 2:43 ` Dan Williams
2021-09-23 5:42 ` Dan Williams
2021-09-23 22:54 ` Dave Chinner
2021-09-24 1:18 ` Dan Williams
2021-09-24 1:21 ` Jane Chu
2021-09-24 1:35 ` Darrick J. Wong
2021-09-27 21:07 ` Dave Chinner
2021-09-27 21:57 ` Jane Chu
2021-09-28 0:08 ` Dan Williams [this message]
2021-09-22 5:28 ` riteshh
2021-09-18 1:31 ` [PATCH 4/5] xfs: implement FALLOC_FL_ZEROINIT_RANGE Darrick J. Wong
2021-09-18 1:31 ` [PATCH 5/5] ext4: " Darrick J. Wong
2021-09-18 17:07 ` riteshh
2021-09-20 18:11 ` Darrick J. Wong
2021-09-21 6:10 ` riteshh
2021-09-18 18:05 ` [PATCHSET RFC v2 jane 0/5] vfs: enable userspace to reset damaged file storage Dan Williams
2021-09-23 0:51 ` Darrick J. Wong
2021-09-23 1:17 ` Dan Williams
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=CAPcyv4gys1F6G1cgXk2UOcr27GNBAfc+ZBoh7MAwFVu5cqfXDg@mail.gmail.com \
--to=dan.j.williams@intel.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=hch@infradead.org \
--cc=jane.chu@oracle.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/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).