[Top] [All Lists]

Re: [PATCH 2/2] xfs: add a test for unlinked inodes due to O_TMPFILE

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: add a test for unlinked inodes due to O_TMPFILE
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 4 Mar 2014 13:04:17 -0800
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Zhi Yong Wu <zwu.kernel@xxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140304203927.GG6851@dastard>
References: <20140304172103.GA24528@xxxxxxxxxxxxx> <20140304172148.GC24528@xxxxxxxxxxxxx> <20140304203927.GG6851@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 05, 2014 at 07:39:27AM +1100, Dave Chinner wrote:
> > +_supported_fs generic
> XFS only.

Hmm.  I remember fixing this up, but for some reason it didn't make it
into the final patch.

> > +# test creating a r/w tmpfile, do I/O and link it into the namespace
> > +$XFS_IO_PROG -x -T \
> > +   -c "pwrite 0 4096" \
> > +   -c "pread 0 4096" \
> > +   -c "freeze" \
> > +   -c "thaw" \
> > +   -c "shutdown" \
> > +   ${SCRATCH_MNT} | _filter_xfs_io
> $testfile?

No, O_TMPFILE doesn't take an actual file name but a "virtual" parent
directory.  It's a reall creative abuse of the open ABI..

> Also, I don't see the file being linked into the namespace, so the
> comment is probably wrong. Also, please add a comment as to
> why the freeze/thaw is necessary.

Yes, we don't want to link it so that we have it on the unlinke dinode
list.  The freeze/thaw is to make sure the log has been cleaned, I'll
add a comment explaining it.

> There it is, but is moving it to lost+found the right thing to do,
> given that it was on the unlinked list and should have had a zero
> link count? i.e. aren't we supposed to free unlinked inodes with a
> zero link count, not recover them to lost+found?

> Yeah, that seems like the wrong behaviour to have for an anonymous
> O_TMPFILE file - it's making it visible because we moved it to
> lost+found in phase 6....

Good question.  I thought about this a little and decided that it wasn't
worth special casing O_TMPFILE inodes in repair, but thinking about it a
bit more this also happens for normal unlinked but open files.  I can
look into this if you want, and would create another test for that case.

> Also, I don't see any icount mismatch, so the comment above in the
> test is probably wrong.

We do have an icount mismatch, but _filter_repair filters it away.

<Prev in Thread] Current Thread [Next in Thread>