Re: [GIT PULL] afs fixes

[pr-tracker-bot]

The pull request you sent on Thu, 29 Oct 2020 14:07:26 +:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-fixes-20201029

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/598a597636f8618a0520fd3ccefedaed9e4709b0

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] afs: Fixes for bugs found by xfstests

[pr-tracker-bot]

The pull request you sent on Tue, 16 Jun 2020 22:51:46 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-fixes-20200616

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/26c20ffcb5c86eb6a052e0d528396de5600c9710

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] afs: Fixes

[pr-tracker-bot]

The pull request you sent on Thu, 22 Aug 2019 14:10:39 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-fixes-20190822

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e8c3fa9f4d3b9c56ee9436c310318a8165d695c1

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] afs: Fixes

[pr-tracker-bot]

The pull request you sent on Wed, 14 Aug 2019 15:18:40 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-fixes-20190814

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e22a97a2a85d2a0bdfb134cbbc7ff856ae67edba

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] AFS fixes

[pr-tracker-bot]

The pull request you sent on Wed, 26 Jun 2019 14:39:53 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-fixes-20190620

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/cd0f3aaebc5b17e0ccb1b9ef9ae43042d075d767

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] AFS fixes

[David Howells]

Oops... I forgot to include the pull request bit.  Will resend.

David

Re: [GIT PULL] AFS fixes and development

[pr-tracker-bot]

The pull request you sent on Tue, 07 May 2019 21:11:17 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-next-20190507

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e5fef2a9732580c5bd30c0097f5e9091a3d58ce5

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] afs: Fixes

[pr-tracker-bot]

The pull request you sent on Thu, 18 Apr 2019 10:17:32 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git 
> tags/afs-fixes-20190413

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/2a852fd1ac893d75879923025306f146b7e0747e

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] AFS fixes and other bits

[David Howells]

Linus Torvalds  wrote:

> The thing hasn't even seen a compiler, and when you *do* show the code
> to a compiler, said compiler correctly warns about

No, it doesn't - at least not gcc-8.3.1 on Fedora 29.  What compiler are you
using?

> afs_do_silly_unlink() potentially returning an uninitialized variable.

Not only has it seen a compiler, it's also been tested numerous times, with
firefox, sqlite and some commands that tests file locking manually.

But please hold off for now.  I've just tried the system flock program - and
that causes a lock leakage message for some reason that needs investigating.

David

Re: [GIT PULL] AFS fixes and other bits

[Linus Torvalds]

On Fri, Mar 15, 2019 at 4:41 PM David Howells  wrote:
>
> Here's a set of fixes and other bits for AFS to improve the life of desktop
> applications such as firefox.

I pulled, and immediately unpulled.

The thing hasn't even seen a compiler, and when you *do* show the code
to a compiler, said compiler correctly warns about
afs_do_silly_unlink() potentially returning an uninitialized variable.

And yes, it's _trivially_ and obviously uninitialized.

Feel free to submit this for 5.2 after it has actually seen testing.
But this late in the 5.1 merge window, I'm no longer interested in
totally untested new crap. It clearly wasn't ready before the merge
window, and it clearly isn't ready *now*.

Much too late to try to fix this up,

  Linus

Re: [GIT PULL] afs: Fixes

[David Howells]

Linus Torvalds  wrote:

> However, even when you do that, the page can be writable in other
> mappings. At least fork(), for example, only clears the dirty bit,
> doesn't mark it write-protected.

I assumed the rmap walk done by page_mkclean() would take care of that but I'm
not really clear on what the code does.

> I just hope that the inconsistency isn't fatal to the afs client or
> server code. For example, if you retry writes forever when a checksum
> were to not match the data, that would be bad.

Shouldn't be a problem for the the in-Linux client.  Data is copied into
sk_bufs preparatory to doing further things to it like checksumming,
encryption or transmission (actually, in future, I would like to use the
encryption process to save on the copy, but this shouldn't bother that
either).

AFAIK, the servers are all userspace jobs that don't let anyone else touch
their storage so that they can maintain correctness on the data version number
of each vnode.

> so I just wanted to bring this up as a potential issue, not
> necessarily as a big problem.

Thanks.

David

Re: [GIT PULL] afs: Fixes

[David Howells]

Linus Torvalds  wrote:

> However, even when you do that, the page can be writable in other
> mappings. At least fork(), for example, only clears the dirty bit,
> doesn't mark it write-protected.

I assumed the rmap walk done by page_mkclean() would take care of that but I'm
not really clear on what the code does.

> I just hope that the inconsistency isn't fatal to the afs client or
> server code. For example, if you retry writes forever when a checksum
> were to not match the data, that would be bad.

Shouldn't be a problem for the the in-Linux client.  Data is copied into
sk_bufs preparatory to doing further things to it like checksumming,
encryption or transmission (actually, in future, I would like to use the
encryption process to save on the copy, but this shouldn't bother that
either).

AFAIK, the servers are all userspace jobs that don't let anyone else touch
their storage so that they can maintain correctness on the data version number
of each vnode.

> so I just wanted to bring this up as a potential issue, not
> necessarily as a big problem.

Thanks.

David

Re: [GIT PULL] afs: Fixes

[Linus Torvalds]

On Sat, Nov 25, 2017 at 12:35 PM, David Howells  wrote:
>
> Doesn't clear_page_dirty_for_io() write-protect the PTE for the page to be
> written out, in which case ->page_mkwrite() will get called again before the
> page is redirtied?

No, it literally just sets the dirty bit (and does accounting).

But I think you may be right that we always write-protect he page when
we move the dirty bit from the page tables to the 'struct page'
(page_mkclean_one()).

However, even when you do that, the page can be writable in other
mappings. At least fork(), for example, only clears the dirty bit,
doesn't mark it write-protected.

So there is some rate-limiting of dirty pages, but I do not believe
that we've ever really *serialized* writes.

>>  (b) can cause some really nasty latency issues
>
> True, but I think the most common case is a file being opened, written start
> to finish and then closed.  Actually, the worst-handled thing I've seen is a
> shell script appending a bunch of things to a file because ->flush() syncs the
> file each time it is closed:-/
>
> What would you recommend instead?  I'm currently trying and keep track of what
> needs to be written so that I only write what's changed to the server, rather
> than writing only whole pages.

I don't think that what you are doing is necessarily wrong, I'm just
pointing out that you may still see mmap'ed pages being modified
concurrently with the actual IO, and that this will potentially mean
(for example) that things like checksums won't be reliably unless you
do the checksum as you copy the data to a network packet or something.

Of course, if that kind of inconsistency happens, a later write-back
will also happen, and eventually fix it. So the server may see
temporarily 'wrong' data, but it won't be final.

I just hope that the inconsistency isn't fatal to the afs client or
server code. For example, if you retry writes forever when a checksum
were to not match the data, that would be bad.

And yes, this can be

 (a) really hard  to trigger in practice

 (b) very hard to debug due to a combination of very specific timing
and behavior.

so I just wanted to bring this up as a potential issue, not
necessarily as a big problem.

 Linus

Re: [GIT PULL] afs: Fixes

[Linus Torvalds]

On Sat, Nov 25, 2017 at 12:35 PM, David Howells  wrote:
>
> Doesn't clear_page_dirty_for_io() write-protect the PTE for the page to be
> written out, in which case ->page_mkwrite() will get called again before the
> page is redirtied?

No, it literally just sets the dirty bit (and does accounting).

But I think you may be right that we always write-protect he page when
we move the dirty bit from the page tables to the 'struct page'
(page_mkclean_one()).

However, even when you do that, the page can be writable in other
mappings. At least fork(), for example, only clears the dirty bit,
doesn't mark it write-protected.

So there is some rate-limiting of dirty pages, but I do not believe
that we've ever really *serialized* writes.

>>  (b) can cause some really nasty latency issues
>
> True, but I think the most common case is a file being opened, written start
> to finish and then closed.  Actually, the worst-handled thing I've seen is a
> shell script appending a bunch of things to a file because ->flush() syncs the
> file each time it is closed:-/
>
> What would you recommend instead?  I'm currently trying and keep track of what
> needs to be written so that I only write what's changed to the server, rather
> than writing only whole pages.

I don't think that what you are doing is necessarily wrong, I'm just
pointing out that you may still see mmap'ed pages being modified
concurrently with the actual IO, and that this will potentially mean
(for example) that things like checksums won't be reliably unless you
do the checksum as you copy the data to a network packet or something.

Of course, if that kind of inconsistency happens, a later write-back
will also happen, and eventually fix it. So the server may see
temporarily 'wrong' data, but it won't be final.

I just hope that the inconsistency isn't fatal to the afs client or
server code. For example, if you retry writes forever when a checksum
were to not match the data, that would be bad.

And yes, this can be

 (a) really hard  to trigger in practice

 (b) very hard to debug due to a combination of very specific timing
and behavior.

so I just wanted to bring this up as a potential issue, not
necessarily as a big problem.

 Linus

Re: [GIT PULL] afs: Fixes

[Dave Chinner]

On Sat, Nov 25, 2017 at 10:35:43PM +, David Howells wrote:
> Linus Torvalds  wrote:
> 
> > So I see in the commit message why afs needs to do this, but it's
> > worth pointing out that it's
> > 
> >  (a) impossible to avoid the "inconsistent data" case for writable mmap'ed
> >  pages
> 
> Doesn't clear_page_dirty_for_io() write-protect the PTE for the page to be
> written out, in which case ->page_mkwrite() will get called again before the
> page is redirtied?

Yes, but page_mkwrite will only block on writeback in progress is if
the backing device says it needs stable pages.  See
wait_for_stable_page().  e.g. stable pages are required if RAID is
in use, otherwise modification during IO can result in broken
on-disk parity/mirroring

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [GIT PULL] afs: Fixes

[Dave Chinner]

On Sat, Nov 25, 2017 at 10:35:43PM +, David Howells wrote:
> Linus Torvalds  wrote:
> 
> > So I see in the commit message why afs needs to do this, but it's
> > worth pointing out that it's
> > 
> >  (a) impossible to avoid the "inconsistent data" case for writable mmap'ed
> >  pages
> 
> Doesn't clear_page_dirty_for_io() write-protect the PTE for the page to be
> written out, in which case ->page_mkwrite() will get called again before the
> page is redirtied?

Yes, but page_mkwrite will only block on writeback in progress is if
the backing device says it needs stable pages.  See
wait_for_stable_page().  e.g. stable pages are required if RAID is
in use, otherwise modification during IO can result in broken
on-disk parity/mirroring

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com

Re: [GIT PULL] afs: Fixes

[David Howells]

Linus Torvalds  wrote:

> So I see in the commit message why afs needs to do this, but it's
> worth pointing out that it's
> 
>  (a) impossible to avoid the "inconsistent data" case for writable mmap'ed
>  pages

Doesn't clear_page_dirty_for_io() write-protect the PTE for the page to be
written out, in which case ->page_mkwrite() will get called again before the
page is redirtied?

>  (b) can cause some really nasty latency issues

True, but I think the most common case is a file being opened, written start
to finish and then closed.  Actually, the worst-handled thing I've seen is a
shell script appending a bunch of things to a file because ->flush() syncs the
file each time it is closed:-/

What would you recommend instead?  I'm currently trying and keep track of what
needs to be written so that I only write what's changed to the server, rather
than writing only whole pages.

David

Re: [GIT PULL] afs: Fixes

[David Howells]

Linus Torvalds  wrote:

> So I see in the commit message why afs needs to do this, but it's
> worth pointing out that it's
> 
>  (a) impossible to avoid the "inconsistent data" case for writable mmap'ed
>  pages

Doesn't clear_page_dirty_for_io() write-protect the PTE for the page to be
written out, in which case ->page_mkwrite() will get called again before the
page is redirtied?

>  (b) can cause some really nasty latency issues

True, but I think the most common case is a file being opened, written start
to finish and then closed.  Actually, the worst-handled thing I've seen is a
shell script appending a bunch of things to a file because ->flush() syncs the
file each time it is closed:-/

What would you recommend instead?  I'm currently trying and keep track of what
needs to be written so that I only write what's changed to the server, rather
than writing only whole pages.

David

Re: [GIT PULL] afs: Fixes

[Linus Torvalds]

On Fri, Nov 24, 2017 at 4:22 AM, David Howells  wrote:
>
>  (2) Don't write to a page that's being written out, but wait for it to
>  complete.

So I see in the commit message why afs needs to do this, but it's
worth pointing out that it's

 (a) impossible to avoid the "inconsistent data" case for writable mmap'ed pages

 (b) can cause some really nasty latency issues

So the "page->private" issue does mean that write-vs-writeback clearly
needs that serialization (and that's not an issue for the mapped page
being changed at the same time), but in general filesystem people
should be aware that data under writeback may still be further dirtied
while the writeback is active (and the page marked dirty again). It
can obviously screw with things like checksums etc, and make for
temporarily inconsistent state if the filesystem does those kinds of
things.

Linus

Re: [GIT PULL] afs: Fixes

[Linus Torvalds]

On Fri, Nov 24, 2017 at 4:22 AM, David Howells  wrote:
>
>  (2) Don't write to a page that's being written out, but wait for it to
>  complete.

So I see in the commit message why afs needs to do this, but it's
worth pointing out that it's

 (a) impossible to avoid the "inconsistent data" case for writable mmap'ed pages

 (b) can cause some really nasty latency issues

So the "page->private" issue does mean that write-vs-writeback clearly
needs that serialization (and that's not an issue for the mapped page
being changed at the same time), but in general filesystem people
should be aware that data under writeback may still be further dirtied
while the writeback is active (and the page marked dirty again). It
can obviously screw with things like checksums etc, and make for
temporarily inconsistent state if the filesystem does those kinds of
things.

Linus

Re: [GIT PULL] AFS fixes

[Linus Torvalds]

On Fri, Mar 17, 2017 at 8:29 AM, David Howells  wrote:
>
> Could you pull these fixes to the AFS filesystem in the kernel please?

I took this, but honestly, most of the patches look like "merge
window" patches rather than -rc patches.

Exactly _one_ of the patches was marked for stable. The rest look like
"yes, they are technically bugs, but this is general afs development".

Linus

Re: [GIT PULL] AFS fixes

[Linus Torvalds]

On Fri, Mar 17, 2017 at 8:29 AM, David Howells  wrote:
>
> Could you pull these fixes to the AFS filesystem in the kernel please?

I took this, but honestly, most of the patches look like "merge
window" patches rather than -rc patches.

Exactly _one_ of the patches was marked for stable. The rest look like
"yes, they are technically bugs, but this is general afs development".

Linus