User Details
- User Since
- Oct 24 2014, 7:17 PM (331 w, 4 d)
Yesterday
Sorry for the delayed review. This change looks correct to me.
Sorry for the delayed review. This fix looks correct to me.
Thu, Feb 25
It appears that this change should be MFC'ed to 12.
Wed, Feb 24
Sat, Feb 20
Its a wrap!
Thu, Feb 18
I note that lib/libprocstat/libprocstat.c includes ufs/ufs/inode.h but in fact does not need to do so.
Getting rid of _buf_cluster.h was a lot more work than I expected, but is definitely cleaner and has the added bonus of cleaning up some other cruft as well.
The problem is in lib/libprocstat that wants to read inodes out of the kernel so needs to know their size. So, I agree avoiding _buf_cluster.h is hard. I reluctantly agree with that solution.
Wed, Feb 17
To avoid the _buf_cluster.h file, how about making the inclusion of buf.h in ufs/inode.h and ext2fs/inode.h conditional on #ifdef KERNEL? Both vfs_cluster.c and fuse_node.h already include buf.h so are not an issue. I don't know if msdosfs/denode.h is used outside the kernel, but if so could make inclusion of buf.h conditional on #ifdef KERNEL.
All looks good.
Tue, Feb 16
Overall looks good.
Overall looks good. A few inline comments.
Thanks for rewriting this comment.
I have provided some suggestions for cleanup and/or clarification.
Fri, Feb 12
Jan 30 2021
Jan 26 2021
Jan 16 2021
Jan 12 2021
Jan 7 2021
This change certainly fixes the problem though it write-locks far more than necessary.
Jan 3 2021
Jan 1 2021
Dec 31 2020
Dec 23 2020
No change in actual running, but definitely correct change.
Dec 18 2020
Dec 11 2020
Dec 9 2020
Dec 8 2020
Dec 6 2020
These updates look needed and correct.
Nov 29 2020
Good to go.
Nov 25 2020
The sentiment is correct, but logic fixes noted are needed.
Nov 20 2020
Belatedly, these changes look good and in particular get rid of VOP_SYNC(..., MNT_WAIT).
Nov 17 2020
Nov 16 2020
I have wanted this change for a long time. Thanks for doing it.
Nov 14 2020
Have we reached any conclusions about whether to do any of the ideas suggested in this phabricator thread?
Nov 6 2020
Two questions.
Nov 1 2020
Oct 31 2020
! In D26964#603222, @rlibby wrote:
! In D26964#603106, @mckusick wrote:
I do like your new approach better as it is much clearer what is going on. I think that it may be sufficient to just make the last change in your delta where you switch it only doing the truncation when i_nlink falls to zero. The other actions taken when i_effnlink falls to zero should still be OK to be done then. As before, getting Peter Holm's testing is important.
Okay. I'll post the diff as you suggest, but I don't quite understand. Those other actions seem just to be doing a vn_start_secondary_write(). Is this so that when i_effnlink <= 0 but i_nlink > 0 and we have one of the IN_ change flags, that we use V_NOWAIT for the vn_start_secondary_write and possibly defer with VI_OWEINACT vs the V_WAIT we might use just above UFS_UPDATE?
My recollection is that the i_effnlink <= 0 when i_nlink > 0 got added because we were somehow missing getting VI_OWEINACT set. But looking through the code, I just cannot come up with a scenario where that is the case. So your original proposed change is probably correct and would save some extra unnecessary work.
This is an impressive piece of work, what a lot of effort to fix this LOR. Took a couple of hours to review, but overall looks good. A couple of minor inline comments.
Oct 30 2020
Oct 27 2020
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.
Oct 25 2020
Oct 23 2020
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.
Oct 18 2020
Added a couple of inline comments.
Oct 3 2020
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.