linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Richard Narron <comet.berkeley@gmail.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [git pull] first batch of ufs fixes
Date: Fri, 16 Jun 2017 07:29:00 -0700 (PDT)	[thread overview]
Message-ID: <alpine.LNX.2.21.1706160655260.1877@joy.test> (raw)
In-Reply-To: <20170615080009.GE31671@ZenIV.linux.org.uk>

On Thu, 15 Jun 2017, Al Viro wrote:

> On Wed, Jun 14, 2017 at 08:11:33AM +0100, Al Viro wrote:
> FWIW, it seems to work here.  Said that, *BSD fsck_ffs is not worth much -
> play a bit with redundancy in UFS superblock (starting with fragment
> and block sizes, their ratio, logarithms, bitmasks, etc.) and you can
> screw at least 10.3 into the ground when mounting an image that passes
> their fsck.  Sure, anyone who mounts untrusted images is a cretin who
> deserves everything they get, fsck or no fsck, but... no complaints from
> fsck is not a reliable indicator of image being in good condition and
> that's PITA for testing.
>
> Another pile of fun: "reserve ->s_minfree percents of total" logics had
> been broken.
> 	* using hardwired 5% is wrong - especially for ufs2, where it's
> not even the default
> 	* ufs_freespace() returns u64; testing for <= 0 is not doing the
> right thing
> 	* no capability checks before we need them, TYVM...
> 	* ufs2 needs 64bit uspi->s_dsize (and ->s_size, while we are at it).
> 64bit variants were even calculated - and never used.
> 	* while we are at it, doing "multiply the total data frags by
> s_minfree and divide by 100" every time we allocate a block is bloody
> dumb - that should be calculated once.
>
> We really need to get the sodding tail unpacking moved up from the place
> where it's buried - turns out that my doubts about that code managing to
> avoid deadlocks had been correct.  Long-term we need to move that thing
> to iomap-based ->write_iter() and do unpacking there and in truncate().
> For now I've slapped together something that is easier to backport -
> avoiding ->truncate_mutex when possible and not holding ->s_lock over
> ufs_change_blocknr().
>
> Another bug in the same area: ufs_get_locked_page() doesn't guarantee
> that buffer_heads are attached (race with vmscan trying to evict the
> page in question can end with buffer_heads freed and page left alive
> and uptodate).  Callers do expect buffer_heads to be there, so we either
> need to do create_empty_buffers() in those callers or in ufs_get_locked_page();
> I went for the latter for now.
>
> Off-by-one in ufs_truncate_blocks(): the logics when deciding whether
> we need to do anything with direct blocks is broken when new size is
> within the last direct block.  It's better to find the path to the
> last byte _not_ to be removed and use that instead of the path to the
> beginning of the first block to be freed.
>
> I've pushed fixes for those into vfs.git#ufs-fixes; they do need more
> testing before I send a pull request, though.

The 8 patches in the ufs-fixes group were applied to Linux 4.12-rc5.
They seem to work fine with the simple testing that I do.

I tested all 3 BSDs, FreeBSD 11.0, OpenBSD 6.1 and NetBSD 7.1 using 2 
filesystems, 44bsd (ufs1) and ufs2.
I found no errors doing a Linux mkdir, copy large file, BSD fsck, Linux rm 
large file, rmdir and BSD fsck in any of the 6 combinations.

Doing a "df" on BSD and Linux now match on the counts including the 
"Available" counts.

It might be worth testing with ufs filesystems using softdep and/or 
journaling.  Should the Linux mount command reject such filesystems?

Now that ufs write access is working more or less, we're dangerous.

  reply	other threads:[~2017-06-16 14:29 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-09 21:38 [git pull] first batch of ufs fixes Al Viro
2017-06-10 13:03 ` Richard Narron
2017-06-10 16:07   ` Al Viro
2017-06-10 18:08     ` Al Viro
2017-06-10 18:12       ` Linus Torvalds
2017-06-11 19:47       ` Richard Narron
2017-06-11 21:30         ` Al Viro
2017-06-12  6:14         ` Al Viro
2017-06-13  0:54           ` Richard Narron
2017-06-13  1:43             ` Al Viro
2017-06-13 21:56               ` Richard Narron
2017-06-14  7:11                 ` Al Viro
2017-06-14 20:33                   ` Richard Narron
2017-06-15  8:00                   ` Al Viro
2017-06-16 14:29                     ` Richard Narron [this message]
2017-06-17  2:15                       ` Al Viro
2017-06-18  1:09                         ` Al Viro
2017-06-18 20:45                           ` Richard Narron
2017-06-20  5:17                           ` [git pull] " Al Viro

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=alpine.LNX.2.21.1706160655260.1877@joy.test \
    --to=comet.berkeley@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ZenIV.linux.org.uk \
    /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).