Anna Schumaker [Wed, 20 Jul 2016 05:31:54 +0000 (15:31 +1000)]
xfs_io: implement 'copy_range' command
Implements a new xfs_io command, named 'copy_range', which is supposed
to be used to copy a range of data from one file to another.
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Zorro Lang [Wed, 20 Jul 2016 05:31:18 +0000 (15:31 +1000)]
xfs_repair: low memory shouldn't indicate corruption on exit
When I run "xfs_repair -n" on a 500T device with 16G memory,
xfs_repair print warning as below:
Memory available for repair (11798MB) may not be sufficient.
At least 64048MB is needed to repair this filesystem efficiently
If repair fails due to lack of memory, please
turn prefetching off (-P) to reduce the memory footprint.
And it returned an exit value of 1. But xfs_repair didn't hit any
error, so there is no reason to mark the fs as corrupted just
because it thinks it might *possibly* not have enough memory to run
to completion.
do_warn() will set fs_is_dirty=1 and hence give a non-zero exit
status. If we only want to print an informational message (not a
real issue), then we should use do_log() instead.
Signed-off-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
kernel commit 5ef828c4
xfs: avoid false quotacheck after unclean shutdown
made xfs_sb_from_disk() also call xfs_sb_quota_from_disk
by default.
However, when this was merged to libxfs, existing separate
calls to libxfs_sb_quota_from_disk remained, and calling it
twice in a row on a V4 superblock leads to issues, because:
and after the second call, we have set both pquotino and gquotino
to NULLFSINO.
Fix this by making it safe to call twice, and also remove the extra
calls to libxfs_sb_quota_from_disk.
This is only spotted when running xfstests with "-m crc=0" because
the sb_from_disk change came about after V5 became default, and
the above behavior only exists on a V4 superblock.
Reported-by: Eryu Guan <eguan@redhat.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Coverity complains that when multiplying two 32 bit values that
eventually will be stored in a 64 bit value that it's possible
the math could overflow unless one of the values being multiplied
is type cast to the proper size.
Signed-off-by: Bill O'Donnell <billodo@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Wed, 29 Jun 2016 01:13:02 +0000 (11:13 +1000)]
libxfs: fix double free in libxfs_alloc_file_space
When porting the transaction alocation interface to userspace
(commit 9074815), I missed a change in libxfs_alloc_file_space() that
could lead to a double free of a transaction pointer in an error path.
Coverity spotted it, so fix it.
Coverity-id: 1362811 Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Wed, 29 Jun 2016 01:12:48 +0000 (11:12 +1000)]
libxfs: fix use after from in xfs_trans_roll
When porting the transaction alocation interface to userspace
(commit 9074815), I missed a change in xfs_trans_roll() that could
lead to a use after free. Coverity spotted it, so fix it.
Coverity-id: 1362812 Signed-off-by: Dave Chinner <david@fromorbit.com>
This goes over the whole indirection array and calls
xfs_iext_irec_remove for each one of the erps (from the last one to
the first one). As a result, we keep shrinking (reallocating
actually) the indirection array until we shrink out all of its
elements. When we have files with huge numbers of extents, umount
takes 30-80 sec, depending on the amount of files that XFS loaded
and the amount of indirection entries of each file. The unmount
stack looks like:
Further, this reallocation prevents us from freeing the extent list
from a RCU callback as allocation can block. Hence if the extent
list is in indirect format, optimise the freeing of the extent list
to only use kmem_free calls by freeing entire extent buffer pages at
a time, rather than extent by extent.
[dchinner: simplified freeing loop based on Christoph's suggestion]
Signed-off-by: Alex Lyakas <alex@zadarastorage.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Use krealloc to implement our realloc function. This helps to avoid
new allocations if we are still in the slab bucket. At least for the
bmap btree root that's actually the common case.
This also allows removing the now unused oldsize argument.
Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Merge xfs_trans_reserve and xfs_trans_alloc into a single function call
that returns a transaction with all the required log and block reservations,
and which allows passing transaction flags directly to avoid the cumbersome
_xfs_trans_alloc interface.
While we're at it we also get rid of the transaction type argument that has
been superflous since we stopped supporting the non-CIL logging mode. The
guts of it will be removed in another patch.
[dchinner: fixed transaction leak in error path in xfs_setattr_nonsize]
Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
By overallocating the in-core inode fork data buffer and zero
terminating the link target in xfs_init_local_fork we can avoid
the memory allocation in ->follow_link.
Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
PAGE_CACHE_{SIZE,SHIFT,MASK,ALIGN} macros were introduced *long* time
ago with promise that one day it will be possible to implement page
cache with bigger chunks than PAGE_SIZE.
This promise never materialized. And unlikely will.
[....]
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Acked-by: Michal Hocko <mhocko@suse.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Dave Chinner <david@fromorbit.com>
xfs_dir2_node_trim_free can return with setting the rvalp argument
pointer. Initialize it to 0 at the beginning of the function and
only update it to 1 if we succeeded trimming a freespace block.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
xfs_bmap_del_extent() handles extent removal from the in-core and
on-disk extent lists. When removing a delalloc range, it updates the
indirect block reservation appropriately based on the removal. It
currently enforces that the new indirect block reservation is less than
or equal to the original. This is normally the case in all situations
except for in certain cases when the removed range creates a hole in a
single delalloc extent, thus splitting a single delalloc extent in two.
It is possible with small enough extents to split an indlen==1 extent
into two such slightly smaller extents. This leaves one extent with 0
indirect blocks and leads to assert failures in other areas (e.g.,
xfs_bunmapi() if the extent happens to be removed).
Update the indlen distribution code to steal blocks from the deleted
extent, if necessary, to satisfy the worst case total indirect
reservation for the new extents. This is safe as the caller does not
update the fdblocks counters until the extent is removed. Blocks stolen
in this manner simply remain accounted as allocated, having ownership
transferred from the data extent to an indirect reservation.
As a precaution, fall back to the original reservation algorithm if the
new indlen requirement is not met and warn if we end up with extents
without any reservation at all to detect this more easily in the future.
Signed-off-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
The delayed allocation indirect reservation splitting code is not
sufficient in some cases where a delalloc extent is split in two. In
preparation for enhancements to this code, refactor the current indlen
distribution algorithm into a new helper function.
[dchinner: rename temp, temp2 variables]
Signed-off-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
xfs_bunmapi() currently updates the fdblocks counter, unreserves quota,
etc. before the extent is deleted by xfs_bmap_del_extent(). The function
has problems dividing up the indirect reserved blocks for scenarios
where a single delalloc extent is split in two. Particularly, there
aren't always enough blocks reserved for multiple extents in a single
extent reservation.
The solution to this problem is to allow the extent removal code to
steal from the deleted extent to meet indirect reservation requirements.
Move the block of code in xfs_bmapi() that updates the fdblocks counter
to after the call to xfs_bmap_del_extent() to allow the codepath to
update the extent record before the free blocks are accounted. Also,
reshuffle the code slightly so the delalloc accounting occurs near the
xfs_bmap_del_extent() call to provide context for the comments.
Signed-off-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
bp_release is set to 0 just before the breakpoint of the for loop before
the conditional check (in line 458). The other breakpoint is a goto that
skips the dead code.
Addresses-Coverity-Id: 102338
Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
difflibxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index ef00156..9054c50 100644
Commit 88740da18[1] introduced a function to compute the maximum
height of the inode btree back in 1994. Back then, apparently, the
freespace and inode btrees shared the same geometry; however, it has
long since been the case that the inode and freespace btrees have
different record and key sizes. Therefore, we must use m_inobt_mnr if
we want a correct calculation/log reservation/etc.
(Yes, this bug has been around for 21 years and ten months.)
(Yes, I was in middle school when this bug was committed.)
Historical-research-by: Dave Chinner <david@fromorbit.com> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Brian Foster [Tue, 21 Jun 2016 02:59:57 +0000 (12:59 +1000)]
xfs_check: process sparse inode chunks correctly
Update the inode btree scanning functions to process sparse inode chunks
correctly. For filesystems with sparse inode support enabled, process
each chunk a cluster at a time. Each cluster is checked against the
inobt record to determine if it is a hole and skipped if so.
Note that since xfs_check is deprecated in favor of xfs_repair, this
adds the minimum support necessary to process sparse inode enabled
filesystems. In other words, this adds no sparse inode specific checks
or verifications. We only update the inobt scanning functions to extend
the existing level of verification to sparse inode enabled filesystems
(e.g., avoid incorrectly tracking sparse regions as inodes). Problems
or corruptions associated with sparse inode records must be detected and
recovered via xfs_repair.
Signed-off-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Brian Foster [Tue, 21 Jun 2016 02:58:57 +0000 (12:58 +1000)]
xfs_db: Revert "xfs_db: make check work for sparse inodes"
This reverts commit bb2f98b78f20f4abbfbbd442162d9f535c84888a which
introduced support for multi-record inode chunks in
xfs_db/xfs_check. However, it doesn't currently handle filesystems
with multi-record inode chunks correctly. For example, do the
following on a 64k page size arch such as ppc64:
# mkfs.xfs -f -b size=64k <dev>
# xfs_db -c check <dev>
bad magic number 0 for inode 1152
bad magic number 0 for inode 1153
bad magic number 0 for inode 1154
bad magic number 0 for inode 1155
bad magic number 0 for inode 1156
bad magic number 0 for inode 1157
...
This boils down to a regression in the inode record processing code
(scanfunc_ino()) in db/check.c. Specifically, the cblocks value can
end up being zero after it is shifted by mp->m_sb.sb_inopblog (i.e.,
64 >> 7 == 0 for an -isize=512 -bsize=64k fs).
Fixing this problem is easier to do from scratch, so revert the
oringial commit first.
Signed-off-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Eric Sandeen [Tue, 21 Jun 2016 02:55:15 +0000 (12:55 +1000)]
xfs_repair: set rsumino version to 2
If we run xfs/033 with "-m crc=0", the test fails with a repair
output difference:
Phase 7 - verify and correct link counts...
+resetting inode INO nlinks from 0 to 1
done
This is because when we zero out the realtime summary inode and
rebuild it, we set its version to 1, then set its ip->i_d.di_nlink
to 1. This is a little odd, because v1 inodes store their link
count in di_onlink...
Then, later in repair we call xfs_inode_from_disk(), which sees the
version one inode, and converts it to version 2 in part by copying
di_onlink to di_nlink. But we never *set* di_onlink, so di_nlink
gets reset to zero, and this error is discovered later in repair.
Interestingly, mk_rbmino() was changed in 138659f1 to set version 2;
it looks like mk_rsumino was just missed.
Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Jan Tulak [Tue, 21 Jun 2016 02:54:30 +0000 (12:54 +1000)]
mkfs: test that -l su is a multiple of block size
lsunit was already tested, but lsu was not. So a thing like -l su=4097 was
possible. This commit adds a check to catch this, and moves the entire
lsu/lsunit block size testing to calc_stripe_factors(), where already is some
logic w.r.t. lsu/lsunit.
Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Jan Tulak [Tue, 21 Jun 2016 02:53:32 +0000 (12:53 +1000)]
mkfs: better error with incorrect b/s value suffix usage
If user writes a value using b or s suffix without explicitly stating the size
of blocks or sectors, mkfs ends with a not helpful error about the value being
too small. It happens because we read the physical geometry after all options
are parsed.
So, tell the user exactly what is wrong with the input.
Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Jan Tulak [Tue, 21 Jun 2016 02:52:22 +0000 (12:52 +1000)]
mkfs: fix -l su minval
-l su should be in range BBTOB(1) <= L_SU <= XLOG_MAX_RECORD_BSIZE,
because the upper limit is imposed by kernel on iclogbuf: stripe
unit can't be bigger than the log buffer, but the log buffer can
span multiple stripe units. L_SUNIT is changed in the same way.
Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
xfs.h: define XFS_IOC_FREEZE even if FIFREEZE is defined
And the same for XFS_IOC_THAW. Just because we now have a common
version of the ioctl we still need to provide the old name for it
for anyone using those.
Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Eric Sandeen [Fri, 3 Jun 2016 01:04:15 +0000 (11:04 +1000)]
xfs_quota: only round up timer reporting > 1 day
I was too hasty with:
d1fe6ff xfs_quota: remove extra 30 seconds from time limit reporting
The point of that extra 30s, turns out, was to allow the user
to set a limit, query it, and get back what they just set, if
it is set to more than a day.
Without it, if we set a grace period to i.e. 3 days, and query it
1 second later, the rounding in the time_to_string function returns
"2 days" not "3 days" as it did before, because we are at
2 days 23:59:59 and it essentially applies a floor() for
brevity. I guess this was confusing.
(I've run into this same conundrum on my stove digital timer;
if you set it to 10m, it blinks "10" at you twice so that you
know what you set, then quickly flips to 9 as it counts down).
In some cases, however (and this is the case that prompted the
prior patch), we display a full "XYZ days hh:mm:ss" - we do this
if the verbose flag is set, or if the timer is less than one day.
In these cases, we should not add the 30s, because we are showing
full time resolution to the user.
Reported-by: Zorro Lang <zlang@redhat.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Bill O'Donnell [Fri, 3 Jun 2016 01:04:03 +0000 (11:04 +1000)]
xfs_repair: further improvement on secondary superblock search method
This patch is a further optimization of secondary sb search, in
order to handle non-default geometries. Once again, use a similar
method to find fs geometry as that of xfs_mkfs. Refactor
verify_sb(), creating new sub-function that checks sanity of
agblocks and agcount: verify_sb_blocksize().
If verify_sb_blocksize verifies sane paramters, use found values for
the sb search. Otherwise, try search with default values. If these
faster methods both fail, fall back to original brute force slower
search.
Signed-off-by: Bill O'Donnell <billodo@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Eric Sandeen [Mon, 30 May 2016 02:21:31 +0000 (12:21 +1000)]
xfs_quota: check report_mount return value
The new call to report_mount doesn't check the return value
like every other caller does...
Returning 1 means it printed something; if the terse flag
is used and there is no usage, nothing gets printed.
If we set the NO_HEADER_FLAG anyway, then we won't see
the header for subsequent entries as we expect.
For example, project ID 0 has no usage in this case:
# xfs_quota -x -c "report -a" /mnt/test
Project quota on /mnt/test (/dev/sdb1)
Blocks
Project ID Used Soft Hard Warn/Grace
---------- --------------------------------------------------
#0 0 0 0 00 [--------]
project 2048 4 4 00 [--none--]
So using the terse flag results in no header when it prints
projects with usage:
# xfs_quota -x -c "report -t -a" /mnt/test
Project quota on /mnt/test (/dev/sdb1)
Blocks
Project ID Used Soft Hard Warn/Grace
---------- --------------------------------------------------
project 2048 4 4 00 [--none--]
Addresses-Coverity-Id: 1361552 Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Bill O'Donnell [Mon, 30 May 2016 02:21:26 +0000 (12:21 +1000)]
xfs_repair: new secondary superblock search method
Optimize secondary sb search, using similar method to find
fs geometry as that of xfs_mkfs. If this faster method fails
in finding a secondary sb, fall back to original brute force
slower search.
Signed-off-by: Bill O'Donnell <billodo@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Eric Sandeen [Mon, 30 May 2016 00:35:56 +0000 (10:35 +1000)]
xfs_db: defang frag command
Too many people freak out about this fictitious "fragmentation
factor." As shown in the fact, it is largely meaningless, because
the number approaches 100% extremely quickly for just a few
extents per file.
I thought about removing it altogether, but perhaps a note
about its uselessness, and a more soothing metric (avg extents
per file) might be useful.
Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
The result should be just the single entry being asked for.
Currently this outputs the entire remainder of the array starting at
the given index. This makes it difficult to extract single entry
values.
This occurs because the printing of a flat array of number types
does not take into account the range that is specified on the
command line, which is held in fl->low and fl->high. To make this
work for flat arrays of number types (print function fp_num), change
print_flist() to limit the count of values to be emitted to the
range specified. This now gives:
To further simplify external parsing of single entry values, if only
a single value is requested from the array of fp_num type, don't
print the array index - it's already known. Hence:
This change will take effect on all types of flat number arrays that
are printed. e.g. the range limiting will work for things like the
AGI unlinked list arrays.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Mon, 30 May 2016 00:32:41 +0000 (10:32 +1000)]
xfs_db: allow recalculating CRCs on invalid metadata
Currently we can't write corrupt structures with valid CRCs on v5
filesystems via xfs_db. TO emulate certain types of corruption
result from software bugs in the kernel code, we need this
capability to set up the corrupted state. i.e. corrupt state with a
valid CRC needs to appear on disk.
This requires us to avoid running the verifier that would otherwise
prevent writing corrupt state to disk. To enable this, add the CRC
offset to the type table for different buffers and add a new flag to
the write command to trigger running a CRC calculation base don this
type table. We can then insert the calculated value into the correct
location in the buffer...
Because some objects are not directly buffer based, we can't easily
do this CRC trick. Those object types will be marked as
TYP_NO_CRC_OFF, and as a result will emit an error such as:
# xfs_db -x -c "inode 96" -c "write -d magic 0x4949" /dev/ram0
Cannot recalculate CRCs on this type of object
#
All v4 superblock types are configured this way, as are inode,
dquots and other v5 metadata types that either don't have CRCs or
don't have a fixed offset into a buffer to store their CRC.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Eric Sandeen [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
xfs_db: fix unaligned accesses
Fix 2 unaligned accesses in xfs_db which caused bus errors on
sparc64. Similar treatment was already done in xfs_repair and
xfs_metadump but somehow xfs_db got missed.
Thanks to Anatoly for reminding me that unaligned access is
a thing. ;)
Resolves-oss-bugzilla: #1140 Reported-by: Anatoly Pugachev <matorola@gmail.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
In practice, this works out to 63 indices; sadly 64 are required
to store a 32k metadata chunk, if the filesystem was created with
XFS_MAX_SECTORSIZE. This leads to more sadness later on, as we
index past arrays etc.
For now, just refuse to create a metadump from a 32k sector
filesystem; that's largely just theoretical at this point anyway.
Also check this on mdrestore, and check the lower bound as well;
the AFL fuzzer showed that interesting things happen when the
metadump image claims to contain a sector size of 0.
Oh, and spell "indices" correctly while we're at it.
Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Jan Tulak [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: add optional 'reason' for illegal_option
Allow us to tell the user what exactly is wrong with the specified
options. For example, that the value is too small, instead of just
generic "bad option."
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: don't treat files as though they are block devices
If the device is actually a file, and "-d file" is not specified,
mkfs will try to treat it as a block device and get stuff wrong.
Image files don't necessarily have the same sector sizes as the
block device or filesystem underlying the image file, nor should we
be issuing discard ioctls on image files.
To fix this sanely, only require "-d file" if the device name is
invalid to trigger creation of the file. Otherwise, use stat() to
determine if the device is a file or block device and deal with that
appropriately by setting the "isfile" variables and turning off
direct IO. Then ensure that we check the "isfile" options before
doing things that are specific to block devices.
Other file/blockdev issues fixed:
- use getstr to detect specifying the data device name
twice.
- check file/size/name parameters before anything else.
- overwrite checks need to be done before the image file is
opened and potentially truncated.
- blkid_get_topology() should not be called for image files,
so warn when it is called that way.
- zero_old_xfs_structures() emits a spurious error:
"existing superblock read failed: Success"
when it is run on a truncated image file. Don't warn if we
see this problem on an image file.
- Don't issue discards on image files.
- Use fsync() for image files, not BLKFLSBUF in
platform_flush_device() for Linux.
[ Eric Sandeen <sandeen@redhat.com>: move check for no mkfs target,
other minor fixes. ]
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: add string options to generic parsing
So that string options are correctly detected for conflicts and
respecification, add a getstr() function that modifies the option
tables appropriately.
[ Eric Sandeen <sandeen@redaht.com>: whitespace fixup ]
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: encode conflicts into parsing table
Many options conflict, so we need to specify which options conflict
with each other in a generic manner. We already have a "seen"
variable used for respecification detection, so we can also use this
code conflict detection. Hence add a "conflicts" array to the sub
options parameter definition.
[ Eric Sandeen <sandeen@redhat.com>: remove explicit L_FILE conflict ]
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: merge getnum
getnum() is now only called by getnum_checked(). Move the two
together into a single getnum() function and change all the callers
back to getnum().
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: table based parsing for converted parameters
All the parameters that can be passed as block or sector sizes need
to be passed the block and sector sizes that they should be using
for conversion. For parameter parsing, it is always the same two
variables, so to make things easy just declare them as global
variables so we can avoid needing to pass them to getnum_checked().
We also need to mark these parameters are requiring conversion so
that we don't need to pass this information manually to
getnum_checked(). Further, some of these options are required to
have a power of 2 value, so add optional checking for that as well.
[ Eric Sandeen <sandeen@redhat.com>: fix min/max for "-l su" ]
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: add respecification detection to generic parsing
Add flags to the generic input parameter tables so that
respecification can be detected in a generic manner.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: use getnum_checked for all ranged parameters
Now that getnum_checked can handle min/max checking, use this for
all parameters that take straight numbers and don't require unit
conversions.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:07 +0000 (17:16 +1000)]
mkfs: getbool is redundant
getbool() can be replaced with getnum_checked with appropriate
min/max values set for the boolean variables. Make boolean
arguments consistent - all accept 0 or 1 value now.
[ Eric Sandeen <sandeen@redhat.com>: manpage tidiness ]
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
mkfs: structify input parameter passing
Passing large number of parameters around to number conversion
functions is painful. Add a structure to encapsulate the constant
parameters that are passed, and convert getnum_checked to use it.
This is the first real step towards a table driven option parser.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Yeah, I just asked for a block size of 2^456858480, and it didn't
get rejected. Great, isn't it?
So, factor out the parsing of logarithmic parameters, and pass in
the maximum valid value that they can take. These maximum values
might not be completely accurate (e.g. block/sector sizes will
affect the eventual valid maximum) but we can get rid of all the
overflows and stupidities before we get to fine-grained validity
checking later in mkfs once things like block and sector sizes have
been finalised.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
mkfs: factor boolean option parsing
Many of the options passed to mkfs have boolean options (0 or 1) and
all hand roll the same code and validity checks. Factor these out
into a common function.
Note that the lazy-count option is now changed to match other
booleans in that if you don't specify a value, it reverts to the
default value (on) rather than throwing an error.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
mkfs: validate all input values
Right now, mkfs does a poor job of input validation of values. Many
parameters do not check for trailing garbage and so will pass
obviously invalid values as OK. Some don't even detect completely
invalid values, leaving it for other checks later on to fail due to
a bad value conversion - these tend to rely on atoi() implicitly
returning a sane value when it is passed garbage, and atoi gives no
guarantee of the return value when passed garbage.
Clean all this up by passing all strings that need to be converted
into values into a common function that is called regardless of
whether unit conversion is needed or not. Further, make sure every
conversion is checked for a valid result, and abort the moment an
invalid value is detected.
Get rid of the silly "isdigits(), cvtnum()" calls which don't use
any of the conversion capabilities of cvtnum() because we've already
ensured that there are no conversion units in the string via the
isdigits() call. These can simply be replaced by a standard
strtoll() call followed by checking for no trailing bytes.
Finally, the block size of the filesystem is not known until all
the options have been parsed and we can determine if the default is
to be used. This means any parameter that relies on using conversion
from filesystem block size (the "NNNb" format) requires the block
size to first be specified on the command line so it is known.
Similarly, we make the same rule for specifying counts in sectors.
This is a change from the existing behaviour that assumes sectors
are 512 bytes unless otherwise changed on the command line. This,
unfortunately, leads to complete silliness where you can specify the
sector size as a count of sectors. It also means that you can do
some conversions with 512 byte sector sizes, and others with
whatever was specified on the command line, meaning the mkfs
behaviour changes depending in where in the command line the sector
size is changed....
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
mkfs: Sanitise the superblock feature macros
They are horrible macros that simply obfuscate the code, so
let's factor the code and make them nice functions.
To do this, add a sb_feat_args structure that carries around the
variables rather than a strange assortment of variables. This means
all the default can be clearly defined in a structure
initialisation, and dependent feature bits are easy to check.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
mkfs: sanitise ftype parameter values.
Because passing "-n ftype=2" should fail.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Dave Chinner [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
xfsprogs: use common code for multi-disk detection
Both xfs_repair and mkfs.xfs need to agree on what is a "multidisk:
configuration - mkfs for determining the AG count of the filesystem,
repair for determining how to automatically parallelise it's
execution. This requires a bunch of common defines that both mkfs
and reapir need to share.
In fact, most of the defines in xfs_mkfs.h could be shared with
other programs (i.e. all the defaults mkfs uses) and so it is
simplest to move xfs_mkfs.h to the shared include directory and add
the new defines to it directly.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Jan Tulak <jtulak@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Zorro Lang [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
xfs_quota: print quota id number if the name can't be found
When use GETNEXTQUOTA ioctl to report project quota, it always
report an unexpected quota:
(null) 0 0 0 00 [--------]
The ID 0 store the default quota, even if no one set default quota,
it still have quota accounting, but not enforced. So GETNEXTQUOTA
can find and report this undefined quota.
From this problem, I thought if others' quota name miss, (null) will
be printed too. e.g.
# xfs_quota -xc "limit -u bsoft=300m bhard=400m test" $mnt
# xfs_quota -xc "report -u" $mnt
User ID Used Soft Hard Warn/Grace
---------- --------------------------------------------------
root 0 0 0 00 [--------]
test 0 307200 409600 00 [--------]
# userdel -r test
# xfs_quota -xc "report -u" $mnt
User ID Used Soft Hard Warn/Grace
---------- --------------------------------------------------
root 0 0 0 00 [--------]
(null) 0 307200 409600 00 [--------]
So this problem same with above id 0's problem. To deal with this,
this patch will print id number if the name can't be found.
However, if we use the old GETQUOTA ioctl, it won't print project id
0 quota information if it's not defined. That's different with
GETNEXTQUOTA. For keep consistent, this patch also print project id
0 when use old GETQUOTA.
Signed-off-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Zorro Lang [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
xfs_quota: fully support users and groups beginning with digits
A normal user or group name allow beginning with digits, but xfs_quota
can't create a limit for that user or group. The reason is 'strtoul'
function only translate digits at the beginning, it will ignore
letters after digits.
There's a commit fd537fc50eeade63bbd2a66105f39d04a011a7f5, it try to
fix "xfsprogs: xfs_quota allow user or group names beginning with
digits". But it doesn't effect 'limit' command, so a command likes:
Zorro Lang [Tue, 10 May 2016 07:16:06 +0000 (17:16 +1000)]
xfs_quota: add missed options -D and -P into man page
There're two options in xfsprogs/quota/init.c:init() function, the -D
option is used to set a file to instead of /etc/projects, and the -P
option is used to set a file to instead of /etc/projid. I don't know
these two options when I write xfstests case xfs/133, because
there's no any information about them in xfs_quota and other related
man pages.
Document these options in the xfs_quota man page.
Reported-by: Eryu Guan <eguan@redhat.com> Signed-off-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Bill O'Donnell <billodo@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Zorro Lang [Tue, 10 May 2016 07:16:05 +0000 (17:16 +1000)]
xfs_io: allow mmap command to reserve some free space
When using xfs_io to rewrite LTP's mmap16 testcase for xfstests,
we always hit mremap ENOMEM error. But according to linux commit:
90a8020 vfs: fix data corruption when blocksize < pagesize for mmaped data
The reproducer shouldn't use MREMAP_MAYMOVE flag for mremap(). So we
need a stable method to make mremap can extend space from the original
mapped starting address.
Generally if we try to mremap from the original mapped starting point
in a C program, at first we always do:
The "res_size" is bigger than "real_len". This will help us get a
region between real_len and res_size, which maybe free space. The
truth is we can't guarantee that this free memory will stay free.
But this method is still very helpful for reserve some free space
in short time.
This patch bring in the "res_size" by use a new -s option. If don't use
this option, xfs_io -c "mmap 0 1024" -c "mremap 8192" will hit ENOMEM
error nearly 100%, but if use this option, it nearly always remap
successfully.
Signed-off-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Carlos Maiolino [Mon, 29 Feb 2016 05:03:40 +0000 (16:03 +1100)]
xfs_io: implement 'inode' command
Implements a new xfs_io command, named 'inode', which is supposed to be
used to query information about inode's existence and its physical size
in the filesystem.
Supported options:
Default: -- Return true(1) or false(0) if any inode greater than
32bits has been found in the filesystem
[num] -- Return inode number or 0 if the inode [num] is in use
-n [num] -- Return the next valid inode after [num]
-v -- verbose mode
Display the inode number and its physical size according to the
argument used
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Bill O'Donnell <billodo@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Zorro Lang [Sun, 28 Feb 2016 23:45:53 +0000 (10:45 +1100)]
xfs_quota: modify commands which can't handle multiple types
Some xfs_quota commands can't deal with multiple types together.
For example, if we run "limit -ug ...", one type will overwrite
the other. I find below commands can't handle multiple types:
[quota, limit, timer, warn, dump, restore and quot]
(Although timer and warn command can't support -ugp types until
now, it will in one day.)
For every single $command, I change their ${command}_f function,
${command}_cmd structure and man page.
Signed-off-by: Zorro Lang <zlang@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Move the di_mode value from the xfs_icdinode to the VFS inode, reducing
the xfs_icdinode byte another 2 bytes and collapsing another 2 byte hole
in the structure.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
We can store the di_changecount in the i_version field of the VFS
inode and remove another 8 bytes from the xfs_icdinode.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
The VFS tracks the inode nlink just like the xfs_icdinode. We can
remove the variable from the icdinode and use the VFS inode variable
everywhere, reducing the size of the xfs_icdinode by a further 4
bytes.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
So we don't have to carry an di_onlink variable around anymore, move
the inode conversion from v1 inode format to v2 inode format into
xfs_inode_from_disk(). This means we can remove the di_onlink fields
from the struct xfs_icdinode.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Now that the struct xfs_icdinode is not directly related to the
on-disk format, we can cull things in it we really don't need to
store:
- magic number never changes
- padding is not necessary
- next_unlinked is never used
- inode number is redundant
- uuid is redundant
- lsn is accessed directly from dinode
- inode CRC is only accessed directly from dinode
Hence we can remove these from the struct xfs_icdinode and redirect
the code that uses them to the xfs_dinode appripriately. This
reduces the size of the struct icdinode from 152 bytes to 88 bytes,
and removes a fair chunk of unnecessary code, too.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
The struct xfs_inode has two copies of the current timestamps in it,
one in the vfs inode and one in the struct xfs_icdinode. Now that we
no longer log the struct xfs_icdinode directly, we don't need to
keep the timestamps in this structure. instead we can copy them
straight out of the VFS inode when formatting the inode log item or
the on-disk inode.
This reduces the struct xfs_inode in size by 24 bytes.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
We currently carry around and log an entire inode core in the
struct xfs_inode. A lot of the information in the inode core is
duplicated in the VFS inode, but we cannot remove this duplication
of infomration because the inode core is logged directly in
xfs_inode_item_format().
Add a new function xfs_inode_item_format_core() that copies the
inode core data into a struct xfs_icdinode that is pulled directly
from the log vector buffer. This means we no longer directly
copy the inode core, but copy the structures one member at a time.
This will be slightly less efficient than copying, but will allow us
to remove duplicate and unnecessary items from the struct xfs_inode.
To enable us to do this, call the new structure a xfs_log_dinode,
so that we know it's different to the physical xfs_dinode and the
in-core xfs_icdinode.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
Buffers without verifiers issue runtime warnings on XFS. We don't
have anything we can actually verify in the RT buffers (no CRCs, not
magic numbers, etc), but we still need verifiers to avoid the
warnings.
Add a set of dummy verifier operations for the realtime buffers and
apply them in the appropriate places.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
When logging buffers, we attach a type to them that follows the
buffer all the way into the log and is used to identify the buffer
contents in log recovery. Both the realtime summary buffers and the
bitmap buffers do not have types defined or set, so when we try to
log them we see assert failure:
Fix this by adding buffer log format types for these buffers, and
add identification support into log recovery for them. Only build the log
recovery support if CONFIG_XFS_RT=y - we can't get into log recovery for real
time filesystems if support is not built into the kernel, and this avoids
potential build problems.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
Add code to allow the Q_XGETNEXTQUOTA quotactl to quickly find
all active quotas by examining the quota inode, and skipping
over unallocated or uninitialized regions.
Userspace can then use this interface rather than i.e. a
getpwent() loop when asked to report all active quotas.
Signed-off-by: Eric Sandeen <sandeen@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
In xfs_acl_from_disk, instead of trusting that xfs_acl.acl_cnt is correct,
make sure that the length of the attributes is correct as well. Also, turn
the aclp parameter into a const pointer.
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
The recent change to the readdir locking made in 40194ec ("xfs:
reinstate the ilock in xfs_readdir") for CXFS directory sanity was
probably the wrong thing to do. Deep in the readdir code we
can take page faults in the filldir callback, and so taking a page
fault while holding an inode ilock creates a new set of locking
issues that lockdep warns all over the place about.
The locking order for regular inodes w.r.t. page faults is io_lock
-> pagefault -> mmap_sem -> ilock. The directory readdir code now
triggers ilock -> page fault -> mmap_sem. While we cannot deadlock
at this point, it inverts all the locking patterns that lockdep
normally sees on XFS inodes, and so triggers lockdep. We worked
around this with commit 93a8614 ("xfs: fix directory inode iolock
lockdep false positive"), but that then just moved the lockdep
warning to deeper in the page fault path and triggered on security
inode locks. Fixing the shmem issue there just moved the lockdep
reports somewhere else, and now we are getting false positives from
filesystem freezing annotations getting confused.
Further, if we enter memory reclaim in a readdir path, we now get
lockdep warning about potential deadlocks because the ilock is held
when we enter reclaim. This, again, is different to a regular file
in that we never allow memory reclaim to run while holding the ilock
for regular files. Hence lockdep now throws
ilock->kmalloc->reclaim->ilock warnings.
Basically, the problem is that the ilock is being used to protect
the directory data and the inode metadata, whereas for a regular
file the iolock protects the data and the ilock protects the
metadata. From the VFS perspective, the i_mutex serialises all
accesses to the directory data, and so not holding the ilock for
readdir doesn't matter. The issue is that CXFS doesn't access
directory data via the VFS, so it has no "data serialisaton"
mechanism. Hence we need to hold the IOLOCK in the correct places to
provide this low level directory data access serialisation.
The ilock can then be used just when the extent list needs to be
read, just like we do for regular files. The directory modification
code can take the iolock exclusive when the ilock is also taken,
and this then ensures that readdir is correct excluded while
modifications are in progress.
Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
The node directory lookup code uses a state structure that tracks the
path of buffers used to search for the hash of a filename through the
leaf blocks. When the lookup encounters a block that ends with the
requested hash, but the entry has not yet been found, it must shift over
to the next block and continue looking for the entry (i.e., duplicate
hashes could continue over into the next block). This shift mechanism
involves walking back up and down the state structure, replacing buffers
at the appropriate btree levels as necessary.
When a buffer is replaced, the old buffer is released and the new buffer
read into the active slot in the path structure. Because the buffer is
read directly into the path slot, a buffer read failure can result in
setting a NULL buffer pointer in an active slot. This throws off the
state cleanup code in xfs_dir2_node_lookup(), which expects to release a
buffer from each active slot. Instead, a BUG occurs due to a NULL
pointer dereference:
This has been reproduced via a parallel fsstress and filesystem shutdown
workload in a loop. The shutdown triggers the read error in the
aforementioned codepath and causes the BUG in xfs_dir2_node_lookup().
Update xfs_da3_path_shift() to update the active path slot atomically
with respect to the caller when a buffer is replaced. This ensures that
the caller always sees the old or new buffer in the slot and prevents
the NULL pointer dereference.
Signed-off-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
When we do dquot readahead in log recovery, we do not use a verifier
as the underlying buffer may not have dquots in it. e.g. the
allocation operation hasn't yet been replayed. Hence we do not want
to fail recovery because we detect an operation to be replayed has
not been run yet. This problem was addressed for inodes in commit d891400 ("xfs: inode buffers may not be valid during recovery
readahead") but the problem was not recognised to exist for dquots
and their buffers as the dquot readahead did not have a verifier.
The result of not using a verifier is that when the buffer is then
next read to replay a dquot modification, the dquot buffer verifier
will only be attached to the buffer if *readahead is not complete*.
Hence we can read the buffer, replay the dquot changes and then add
it to the delwri submission list without it having a verifier
attached to it. This then generates warnings in xfs_buf_ioapply(),
which catches and warns about this case.
Fix this and make it handle the same readahead verifier error cases
as for inode buffers by adding a new readahead verifier that has a
write operation as well as a read operation that marks the buffer as
not done if any corruption is detected. Also make sure we don't run
readahead if the dquot buffer has been marked as cancelled by
recovery.
This will result in readahead either succeeding and the buffer
having a valid write verifier, or readahead failing and the buffer
state requiring the subsequent read to resubmit the IO with the new
verifier. In either case, this will result in the buffer always
ending up with a valid write verifier on it.
Note: we also need to fix the inode buffer readahead error handling
to mark the buffer with EIO. Brian noticed the code I copied from
there wrong during review, so fix it at the same time. Add comments
linking the two functions that handle readahead verifier errors
together so we don't forget this behavioural link in future.
cc: <stable@vger.kernel.org> # 3.12 - current Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
When we do inode readahead in log recovery, we do can do the
readahead before we've replayed the icreate transaction that stamps
the buffer with inode cores. The inode readahead verifier catches
this and marks the buffer as !done to indicate that it doesn't yet
contain valid inodes.
In adding buffer error notification (i.e. setting b_error = -EIO at
the same time as as we clear the done flag) to such a readahead
verifier failure, we can then get subsequent inode recovery failing
with this error:
This occurs when readahead completion races with icreate item replay
such as:
inode readahead
find buffer
lock buffer
submit RA io
....
icreate recovery
xfs_trans_get_buffer
find buffer
lock buffer
<blocks on RA completion>
.....
<ra completion>
fails verifier
clear XBF_DONE
set bp->b_error = -EIO
release and unlock buffer
<icreate gains lock>
icreate initialises buffer
marks buffer as done
adds buffer to delayed write queue
releases buffer
At this point, we have an initialised inode buffer that is up to
date but has an -EIO state registered against it. When we finally
get to recovering an inode in that buffer:
inode item recovery
xfs_trans_read_buffer
find buffer
lock buffer
sees XBF_DONE is set, returns buffer
sees bp->b_error is set
fail log recovery!
Essentially, we need xfs_trans_get_buf_map() to clear the error status of
the buffer when doing a lookup. This function returns uninitialised
buffers, so the buffer returned can not be in an error state and
none of the code that uses this function expects b_error to be set
on return. Indeed, there is an ASSERT(!bp->b_error); in the
transaction case in xfs_trans_get_buf_map() that would have caught
this if log recovery used transactions....
This patch firstly changes the inode readahead failure to set -EIO
on the buffer, and secondly changes xfs_buf_get_map() to never
return a buffer with an error state set so this first change doesn't
cause unexpected log recovery failures.
cc: <stable@vger.kernel.org> # 3.12 - current Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com>
projects / thirdparty / xfsprogs-dev.git / log
