Changeset View
Standalone View
sys/fs/msdosfs/msdosfs_fat.c
| Show First 20 Lines • Show All 1,115 Lines • ▼ Show 20 Lines | |||||
| * should be marked clean | * should be marked clean | ||||
| * | * | ||||
| * Result: | * Result: | ||||
| * 0 Success | * 0 Success | ||||
| * EROFS Volume is read-only | * EROFS Volume is read-only | ||||
| * ? (other errors from called routines) | * ? (other errors from called routines) | ||||
| */ | */ | ||||
| int | int | ||||
| markvoldirty(struct msdosfsmount *pmp, int dirty) | markvoldirty_upgrade(struct msdosfsmount *pmp, bool dirty, bool rw_upgrade) | ||||
| { | { | ||||
| struct buf *bp; | struct buf *bp; | ||||
| u_long bn, bo, bsize, byteoffset, fatval; | u_long bn, bo, bsize, byteoffset, fatval; | ||||
| int error; | int error; | ||||
| /* | /* | ||||
| * FAT12 does not support a "clean" bit, so don't do anything for | * FAT12 does not support a "clean" bit, so don't do anything for | ||||
| * FAT12. | * FAT12. | ||||
| */ | */ | ||||
| if (FAT12(pmp)) | if (FAT12(pmp)) | ||||
| return (0); | return (0); | ||||
| /* Can't change the bit on a read-only filesystem. */ | /* | ||||
| if (pmp->pm_flags & MSDOSFSMNT_RONLY) | * Can't change the bit on a read-only filesystem, except as part of | ||||
| * ro->rw upgrade. | |||||
| */ | |||||
| if ((pmp->pm_flags & MSDOSFSMNT_RONLY) != 0 && !rw_upgrade) | |||||
| return (EROFS); | return (EROFS); | ||||
| /* | /* | ||||
| * Fetch the block containing the FAT entry. It is given by the | * Fetch the block containing the FAT entry. It is given by the | ||||
| * pseudo-cluster 1. | * pseudo-cluster 1. | ||||
| */ | */ | ||||
| byteoffset = FATOFS(pmp, 1); | byteoffset = FATOFS(pmp, 1); | ||||
| fatblock(pmp, byteoffset, &bn, &bsize, &bo); | fatblock(pmp, byteoffset, &bn, &bsize, &bo); | ||||
| error = bread(pmp->pm_devvp, bn, bsize, NOCRED, &bp); | error = bread(pmp->pm_devvp, bn, bsize, NOCRED, &bp); | ||||
| if (error) { | if (error) | ||||
| brelse(bp); | |||||
kib: This is unrelated and should be committed separately. | |||||
Done Inline ActionsSure, it was just a trivial cleanup nearby. cem: Sure, it was just a trivial cleanup nearby. | |||||
| return (error); | return (error); | ||||
| } | |||||
| /* | /* | ||||
| * Get the current value of the FAT entry and set/clear the relevant | * Get the current value of the FAT entry and set/clear the relevant | ||||
| * bit. Dirty means clear the "clean" bit; clean means set the | * bit. Dirty means clear the "clean" bit; clean means set the | ||||
| * "clean" bit. | * "clean" bit. | ||||
| */ | */ | ||||
| if (FAT32(pmp)) { | if (FAT32(pmp)) { | ||||
| /* FAT32 uses bit 27. */ | /* FAT32 uses bit 27. */ | ||||
| Show All 9 Lines | if (FAT32(pmp)) { | ||||
| if (dirty) | if (dirty) | ||||
| fatval &= 0x7FFF; | fatval &= 0x7FFF; | ||||
| else | else | ||||
| fatval |= 0x8000; | fatval |= 0x8000; | ||||
| putushort(&bp->b_data[bo], fatval); | putushort(&bp->b_data[bo], fatval); | ||||
| } | } | ||||
| /* Write out the modified FAT block synchronously. */ | /* Write out the modified FAT block synchronously. */ | ||||
| return (bwrite(bp)); | error = bwrite(bp); | ||||
| if (error != 0) { | |||||
| int terror; | |||||
Done Inline ActionsPlease follow style, all declarations should be located at the start of the function. But see below, this variable might be not needed. kib: Please follow style, all declarations should be located at the start of the function. But see… | |||||
| /* | |||||
| * Get the block back and discard it without write to disk. | |||||
| * | |||||
| * If we're now discovering that this media is readonly (the | |||||
| * WriteProtect discovery earlier didn't discover this info) or | |||||
| * just gone, throw away our unimportant attempt to modify the | |||||
| * clean bit and bail higher up the stack (failing mount, or | |||||
| * failing RO -> RW transition, at least). See PR 210316 for | |||||
Not Done Inline ActionsWe do not reference bug numbers in comments, source should be enough to understand. kib: We do not reference bug numbers in comments, source should be enough to understand. | |||||
| * more context. | |||||
| */ | |||||
| terror = getblkx(pmp->pm_devvp, bn, bsize, 0, 0, GB_UNMAPPED, | |||||
Done Inline ActionsI would use incore() instead, to clearly state the intent of getting rid of failed write. The devvp vnode is not locked and in principle this buffer might be processed after bwrite(). kib: I would use incore() instead, to clearly state the intent of getting rid of failed write. The… | |||||
Done Inline ActionsDoesn't geom prevent access by multiple writers? I suppose devvp could be a non-GEOM device. What would it mean if incore() did not find the buffer? The write eventually succeeded through the (?)bufdaemon or some other devvp writer? Is there a reason to prefer incore() over getblk(GB_NOCREAT)? If we use incore we have to pull at least some of the buf locking logic out of getblk into the caller; maybe more. cem: Doesn't geom prevent access by multiple writers? I suppose devvp could be a non-GEOM device. | |||||
Done Inline ActionsI attempted to use incore() here, but bundirty() panics due to the buf still being on QUEUE_DIRTY. I guess this may be as simple as needing bremfree(), another thing getblk() does for me. (Also, it seems incore() has a BO lock -> buf lock race that getblk does not. I don't know if that matters.) cem: I attempted to use `incore()` here, but `bundirty()` panics due to the buf still being on… | |||||
Not Done Inline ActionsFrom geom PoV, multiple writers are entities like VFS consumers, and not threads. Other thread might start modifications to the mount point. The change to add rw_upgrade looks good. kib: From geom PoV, multiple writers are entities like VFS consumers, and not threads. Other thread… | |||||
| &bp); | |||||
| KASSERT(terror == 0, ("getblkx: %d for just written block %lu " | |||||
| "(handling failed write %d)\n", terror, bn, error)); | |||||
| bfinval(bp); | |||||
Not Done Inline ActionsPlease drop the reference to PR in source code. kib: Please drop the reference to PR in source code. | |||||
| } | |||||
| return (error); | |||||
| } | } | ||||
Done Inline ActionsDon't we need to check for the buffer error state before calling bfinval() ? Otherwise we might destroy somebody else work. In fact, after reading the updated comment, I start thinking that neither incore() nor getblkx() are the right interfaces there, since we loose ownership of the buffer. I suggest to add a flag, e.g. B_DIEONERR, which would be processed by brelse() and which cause the same action as bfinval() on BIO_ERROR. Then this fragment would simply set B_DIEONERR before calling bwrite(). kib: Don't we need to check for the buffer error state before calling bfinval() ? Otherwise we… | |||||
Done Inline ActionsHm, would it make sense to instead add a variant of bwrite() that does not brelse the buffer, leaving it to the caller? That way our ownership of the buf is maintained. The approach is a little more flexible than adding another brelse side-effect flag. cem: Hm, would it make sense to instead add a variant of `bwrite()` that does not `brelse` the… | |||||
Done Inline ActionsI do not want yet another bwrite() variant, we already have dozen of them. But I am fine with adding a buffer flag that prevents brelse(). Fine details is that the flag should be mutually exclusive with B_ASYNC. kib: I do not want yet another bwrite() variant, we already have dozen of them. But I am fine with… | |||||
This is unrelated and should be committed separately.