- User Since
- Oct 24 2014, 7:17 PM (313 w, 6 d)
Tue, Oct 27
You note that ``If we then crashed before the dirent write, we would recover with a state where the file was still linked, but had been truncated to zero. The resulting state could be considered corruption.'' The filesystem is not corrupted in the sense that it needs fsck to be run to clean it up. We simply end up with an unexpected result.
Sun, Oct 25
Fri, Oct 23
It would be trivial to request high priority for synchronous writes in bwrite() and if desired synchronous reads in bread(). That would have effects for several filesystems.
Sun, Oct 18
Added a couple of inline comments.
Sat, Oct 3
Sep 26 2020
Sep 25 2020
What we are trying to do where the barrier write is being used in UFS is expand the number of available inodes. It is OK to abandon the write of the zero'ed out inode block as the expansion can be put off to later. But, we must not update the cylinder group to say that these new inodes are available if we have not been able to zero them out. As it is currently written, the cylinder group is updated but the on-disk inodes are not zero'ed out. If the unmount succeeds in writing out the dirty buffers we will end up with a corrupted filesystem because fsck_ffs will try to check the non-zero'ed out inodes and raise numerous errors trying to correct the inconsistencies that arise from the random data in the uninitialzed inode block.
Sep 23 2020
We are depending on this being written before it can be used. If it were unlocked, then some other thread could get it and make use of it. See comment above use of babarrierwrite in sys/ufs/ffs/ffs_alloc.c.
Sep 22 2020
Sep 21 2020
As long as the buffer remains locked until it is successfully written, this should be fine.
Sep 19 2020
Sep 13 2020
Sep 12 2020
This change looks reasonable to me as at least for amd64 every path out of the kernel appears to check TDF_ASTPENDING and calls ast() if set.
Sep 1 2020
It was a horrible hack at the time and should have been tossed decades ago.
Aug 31 2020
Aug 29 2020
A new review? If so, I suggest that you close this one and give a pointer to the new one.
Aug 27 2020
What is the status with this change?
Aug 26 2020
The get_parent_vp() function seems to be handling two different problems.
In its first instance it is dealing with locking its parent. Here we already have the function vn_vget_ino() which presumably could be used to handle this situation.
In its second and third instances it is avoiding a LOR of acquiring an inode lock while holding a buffer lock. Here we need a function like the proposed get_parent_vp(). However, we will no longer need the first (vp) argument as we are acquiring a child inode so do not have to release the already held parent inode before acquiring the child inode.
Aug 15 2020
Aug 8 2020
This change will provide the interface that we will need to notify daemon processes for our forcible unmount and forcible downgrade to read-only as well as re-establishment of read-write after a successful background fsck.
Jul 22 2020
I concur with the proposed change and also agree that cem's suggestion is a good one.
Jul 21 2020
Jul 14 2020
Jul 13 2020
This should not be done as written as it can cause a denial of service (infinite loop) as described in my previous comment.
Jul 10 2020
If data is being written to the filesystem faster than the disk can write it then a sync with MNT_WAIT will never finish. The only safe way to use sync with MNT_WAIT is to first suspend the filesystem to create a finite number of write operations that need to be done.
Jul 9 2020
Jul 6 2020
Jun 30 2020
Per the above commentary, this change should be withdrawn.
Jun 24 2020
Having read Chuck's argument, I agree with it. I was thinking that the g_label_ufs.c routines passed in M_UFSMNT as their malloc type. But they do not. They use M_GEOM malloc type. So there is no need for them to have an definition for M_UFSMNT.
Thanks for this cleanup. I found the previous transformation grating for the reasons that you listed.
This looks like a reasonable change to me.
I believe that the existing test already has this covered. The test (resid > uio->uio_resid) is checking that any data has been written. In the typical case this is at the end of the file and the size will have increased. But if the write was in the middle of a file filling in a hole then this test will catch that and cause the inode to be written.
Jun 23 2020
Jun 19 2020
Interesting that I never added myself to the committers-src.dot file.
For reference, the file that actually controls access to doing src commits is /usr/src/svnadmin/conf/access.
For interesting statistics on the history of commits, fetch these files:
fetch https://people.freebsd.org/~rodrigo/commits-base.txt fetch https://people.freebsd.org/~rodrigo/commits-doc.txt fetch https://people.freebsd.org/~rodrigo/commits-ports.txt
I agree that there should be documentation of MFC guidelines somewhere.
For a change of this sort an MFC delay in the range of 1-2 weeks is reasonable.
Jun 18 2020
Your argument has pursuaded me. Looks good to go.
One other suggestion is to change "normal" to "timeshare" or perhaps "timeshr" as that name is more descriptive of what is being reported.
On further though, I agree with you that ffs_subr.c should only be shared functions.
Jun 16 2020
This looks like a big improvement. My one suggestion is that you add the priority value for normal (as level - PRI_MIN_TIMESHARE).
Rather than moving all these functions from a file in which they logically belong to one in which they do not, how about we just put #ifdef FFS around the _KERNEL part. This produces minimal changes, leaves things where they logically belong, and reduces the size of the extra copy of ffs_subr.o.
Jun 6 2020
Jun 5 2020
Jun 4 2020
This looks good to go.
Jun 3 2020
Overall this looks like a correct change. As noted in my inline comment, I do not think that you should change the semantics of waitfor.
May 19 2020
Finally, this looks good to go.
May 15 2020
I believe that this change is correct as it stands.
Can you run one more test run over this code just to be sure that it patches cleanly and that none of our final changes broke anything?
May 13 2020
This looks good to me. I am fine with not supporting the old behavior.
May 9 2020
May 3 2020
This rather long block is a summary of the discussion that Kostik
and I had in email. Dropping it in here so that it is part of the
public record. Regular font are my comments; italics font are
Kostik's response to my comments.
As imp has noted, we have had readdir(3) and the getdirents(2) for nearly 40 years and many filesystems do not support directly reading directories. While I personally still use od(1) to inspect UFS directories, that is a pretty lame reason for keeping the functionality. And returning EISDIR will make us more compatible with Linux and remain in compliance with POSIX. So, all told I am in favor of this change. Adding a sysctl to revert to the old behavior seems reasonable so long as it defaults to the new (EISDIR) behavior.
Apr 20 2020
I concur with this change.
It should be MFC'ed to stable/12 and stable/11.
Apr 15 2020
Apr 10 2020
Apr 9 2020
Apr 6 2020
Apr 4 2020
Apr 3 2020
Changes moved to D24210.
These changes will be done as part of D24210 so that all the updates to make the dump and restore utilities compile with -fno-common are in one place.
Rather than using EXTERN, place each global declaration into the file in which it is primarily used.
Taking over with Kyle Evans blessing.
Mar 31 2020
Now looks good.
Rather than doing EXTERN, move the explicit declarations into file as this:
mapsize - main.c
usedinomap - main.c
dumpdirmap - main.c
dumpinomap - main.c
disk - main.c
tape - main.c
popenout - main.c
dumpdates - itime.c
temp - unused, remove from dump.h, main.c, and definition of _PATH_DTMP from pathnames.h
lastlevel - itime.c
level - main.c
uflag - main.c
diskfd - main.c
tapefd - tape.c
pipeout - main.c
curino - tape.c
newtape - tape.c
density - already declared in main.c
tapesize - main.c
tsize - main.c
asize - tape.c
etapes - main.c
nonodump - main.c
unlimited - main.c
cachesize - already declared in main.c
rsync_friendly - main.c
notify - already declared in main.c
blockswritten - already declared in main.c
tapeno - already declared in main.c
tstart_writing - main.c
tend_writing - main.c
passno - main.c
sblock - main.c
dev_bsize - already declared in main.c
dev_bshift - main.c
tp_bshift - main.c
dumpdates - leave in itime.c
nddates - leave in itime.c
If you would like me to take over this project, I am willing to do so.
Mar 30 2020
I prefer just using extern and then declaring the actual variables in the function in which they are most prominent, but using EXTERN as you have done does work fine.
While u_spcl is used only in dump/tape.c, it is used in four different files in restore and is not meant to be a separate instance in each one. So, you will have to make it an extern in the header file and then declare it in dump/tape.c and restore/dump.c. I prefer that you do it that way as opposed to the much uglier EXTERN approach.
This looks complete. I had missed the VOP_* manual pages, good catch.
I am happy to have you clean up the duplicate EIOs.
Thanks for doing this.
Mar 24 2020
While read(2) and write(2) are the most likely system calls to return EINTEGRITY, really any system call that accesses a filesystem can potentially return EINTEGRITY. Using the metric that if it can return EIO, then it can now return EINTEGRITY leads to this list of system calls needing EINTEGRITY added: access.2, acct.2, bind.2, chdir.2, chflags.2, chmod.2, chown.2, chroot.2, copy_file_range.2, execve.2, fhlink.2, fhreadlink.2, fsync.2, getdirentries.2, getfh.2, getfsstat.2, intro.2, ktrace.2, link.2, lio_listio.2, mkdir.2, mkfifo.2, mknod.2, mount.2, msync.2, open.2, pathconf.2, posix_fallocate.2, quotactl.2, readlink.2, rename.2, rmdir.2, sendfile.2, stat.2, statfs.2, swapon.2, symlink.2, truncate.2, undelete.2, unlink.2, utimensat.2, and utimes.2.
Mar 20 2020
This is a good example of a place that where EINTEGRITY is intended to be used.
At the moment mount(2) is the only system call that lists EINTEGRITY as a possible error. If your change is going to cause EINTEGRITY to be returned to other system calls (like read(2) or wirite(2)) then you should add it to their list of possible error returns.
Mar 17 2020
With the above suggested changes, I agree with this proposal.
Mar 11 2020
Feb 27 2020
I am happy with this revision.
Feb 18 2020
Jan 28 2020
The logic is easier for me to follow.