All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Richard Narron <comet.berkeley@gmail.com>
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: Sun, 18 Jun 2017 02:09:28 +0100	[thread overview]
Message-ID: <20170618010923.GX31671@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20170617021548.GR31671@ZenIV.linux.org.uk>

On Sat, Jun 17, 2017 at 03:15:48AM +0100, Al Viro wrote:
> On Fri, Jun 16, 2017 at 07:29:00AM -0700, Richard Narron wrote:
> 
> > 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.
> 
> FWIW, with xfstests I see the following:
> 	* _require_metadata_journaling needs to be told not to bother with
> our ufs
> 	* generic/{409,410,411} lack $MOUNT_OPTIONS in several invocations
> of mount -t $FSTYP; for ufs it's obviosuly a problem.  Trivial bug in tests,
> fixing it makes them pass.
> 	* generic/258 (timestamp wraparound) fails; fs is left intact

Trivially fixed (cast to (signed) in ufs1_read_inode(), similar to what
other filesystems with 32bit timestamps are doing); ufs2 has no problem
at all)

> 	* generic/426 (fhandle stuff) fails and buggers the filesystem
> Everything else passes with no fs corruption that could be detected by
> fsck.ufs.

Also trivially fixed - it's a self-inflicted wound.  Just have zero nlink in
ufs{1,2}_read_inode() fail with -ESTALE instead of triggering ufs_error().

> As for my immediate plans, I'll look into the two failing tests,
> but any further active work on ufs will have to wait for the next
> cycle.  It had been a fun couple of weeks, but I have more than
> enough other stuff to deal with.  And I would still very much prefer
> for somebody to adopt that puppy.

Another piece of fun spotted: the logics for switching between two allocation
policies when relocating a packed tail that can't be expanded in place had
been b0rken since typo in 2.4.14.7 - switch back from OPTTIME to OPTSPACE
had been screwed by this:
-               usb1->fs_optim = SWAB32(UFS_OPTSPACE);
+               usb1->fs_optim = cpu_to_fs32(sb, UFS_OPTTIME);

And fragmentation levels for switching back and force really ought to be
calculated at mount time.  Another (minor) issue is mentioned in this
commit message from Kirck McKusick back in 1995:
        The threshold for switching from time-space and space-time is too small
        when minfree is 5%...so make it stay at space in this case.
Not that minfree at 5% had been frequently seen - default has never been that
low (back in 4.2BSD it was 10%, these days it's 8%)

Resulting kernel passes xfstests clean and now I'm definitely done with UFS for
this cycle.  Linus, in case you want to pull that sucker, pull request would
be as below:

The following changes since commit a8fad984833832d5ca11a9ed64ddc55646da30e3:

  ufs_truncate_blocks(): fix the case when size is in the last direct block (2017-06-15 03:57:46 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git ufs-fixes

for you to fetch changes up to 77e9ce327d9b607cd6e57c0f4524a654dc59c4b1:

  ufs: fix the logics for tail relocation (2017-06-17 17:22:42 -0400)

----------------------------------------------------------------
Al Viro (3):
      fix signedness of timestamps on ufs1
      ufs_iget(): fail with -ESTALE on deleted inode
      ufs: fix the logics for tail relocation

 fs/ufs/balloc.c | 22 ++++++----------------
 fs/ufs/inode.c  | 27 +++++++++++----------------
 fs/ufs/super.c  |  9 +++++++++
 fs/ufs/ufs_fs.h |  2 ++
 4 files changed, 28 insertions(+), 32 deletions(-)

  reply	other threads:[~2017-06-18  1:09 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
2017-06-17  2:15                       ` Al Viro
2017-06-18  1:09                         ` Al Viro [this message]
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=20170618010923.GX31671@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=comet.berkeley@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.