On Wed, Feb 12, 2020 at 08:49:17AM +1100, Dave Chinner wrote: > On Tue, Feb 11, 2020 at 12:14:31PM -0800, Ira Weiny wrote: > > On Tue, Feb 11, 2020 at 07:00:35PM +1100, Dave Chinner wrote: > > > On Sat, Feb 08, 2020 at 11:34:40AM -0800, ira.weiny@xxxxxxxxx wrote: > > > > From: Ira Weiny <ira.weiny@xxxxxxxxx> > > > > > > > > DAX requires special address space operations but many other functions > > > > check the IS_DAX() state. > > > > > > > > While DAX is a property of the inode we perfer a lock at the super block > > > > level because of the overhead of a rwsem within the inode. > > > > > > > > Define a vfs per superblock percpu rs semaphore to lock the DAX state > > > > > > ???? > > > > oops... I must have forgotten to update the commit message when I did the > > global RW sem. I implemented the per-SB, percpu rwsem first but it was > > suggested that the percpu nature of the lock combined with the anticipated > > infrequent use of the write side made using a global easier. > > > > But before I proceed on V4 I'd like to get consensus on which of the 2 locking > > models to go with. > > > > 1) percpu per superblock lock > > 2) per inode rwsem > > > > Depending on my mood I can convince myself of both being performant but the > > percpu is very attractive because I don't anticipate many changes of state > > during run time. OTOH concurrent threads accessing the same file at run time > > may also be low so there is likely to be little read contention across CPU's on > > the per-inode lock? > > > > Opinions? > > > > > > > > > while performing various VFS layer operations. Write lock calls are > > > > provided here but are used in subsequent patches by the file systems > > > > themselves. > > > > > > > > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx> > > > > > > > > --- > > > > Changes from V2 > > > > > > > > Rebase on linux-next-08-02-2020 > > > > > > > > Fix locking order > > > > Change all references from mode to state where appropriate > > > > add CONFIG_FS_DAX requirement for state change > > > > Use a static branch to enable locking only when a dax capable > > > > device has been seen. > > > > > > > > Move the lock to a global vfs lock > > > > > > > > this does a few things > > > > 1) preps us better for ext4 support > > > > 2) removes funky callbacks from inode ops > > > > 3) remove complexity from XFS and probably from > > > > ext4 later > > > > > > > > We can do this because > > > > 1) the locking order is required to be at the > > > > highest level anyway, so why complicate xfs > > > > 2) We had to move the sem to the super_block > > > > because it is too heavy for the inode. > > > > 3) After internal discussions with Dan we > > > > decided that this would be easier, just as > > > > performant, and with slightly less overhead > > > > than in the VFS SB. > > > > > > > > We also change the functions names to up/down; > > > > read/write as appropriate. Previous names were over > > > > simplified. > > > > > > This, IMO, is a bit of a train wreck. > > > > > > This patch has nothing to do with "DAX state", it's about > > > serialising access to the aops vector. > > > > It is a bit more than just the aops vector. It also has to protect against > > checks of IS_DAX() which occur through many of the file operations. > > I think you are looking at this incorrectly. The IS_DAX() construct > is just determining what path to the aops method we take. regardless > of whether IS_DAX() is true or not, we need to execute an aop > method, and so aops vector protection is required regardless of how > IS_DAX() evaluates. > > But we do require IS_DAX() to evaluate consistently through an > entire aops protected region, as there may be multiple aops method > calls in a aops context (e.g. write page faults). Hence we have to > ensure that IS_DAX() changes atomically w.r.t. to the aops vector > switches. This is simply: > > sb_aops_lock() > inode->i_flags |= S_DAX > sb_aops_switch(new_aops) > sb_aops_unlock(). > > This guarantees that inside sb_aops_start/end(), IS_DAX() checks > are stable because we change the state atomically with the aops > vector. > > We are *not* providing serialisation of inode->i_flags access or > updates here; all we need to do is ensure that the S_DAX flag is > consistent and stable across an aops access region. If we are not > in an aops call chain or will not call an aops method, we just don't > care if the IS_DAX() call is racy as whatever we call is still > static and if it's DAAX sensitive it can call IS_DAX() itself when > needed. > > Again, this isn't about DAX at all, it's about being able to switch > aops vectors in a safe and reliable manner. The IS_DAX() constraints > are really a minor addition on top of the "stable aops vector" > regions we are laying down here. > > > > > Big problems I see here: > > > > > > 1. static key to turn it on globally. > > > - just a gross hack around doing it properly with a per-sb > > > mechanism and enbaling it only on filesystems that are on DAX > > > capable block devices. > > > > Fair enough. This was a reaction to Darricks desire to get this out of the way > > when DAX was not in the system. The static branch seemed like a good thing for > > users who don't have DAX capable hardware while running kernels and FS's which > > have DAX enabled. > > > > http://lkml.iu.edu/hypermail/linux/kernel/2001.1/05691.html > > I think that's largely premature optimisation, and it backs us into > the "all or nothing" corner which is a bad place to be for what is > per-filesystem functionality. Oh, wow, uh... this was a total misunderstanding. Having a per-inode primitive to grant ourselves the ability to change fops/aops safely was fine, I just wanted to have it compile out of existence if CONFIG_DAX=n (or some other cleverish way if this mount will never support DAX (e.g. scsi disk)). I wasn't asking for it to move to the superblock or become a Big Kernel Primitive. --D > > > > - you're already passing in an inode to all these functions. It's > > > trivial to do: > > > > > > if (!inode->i_sb->s_flags & S_DYNAMIC_AOPS) > > > return > > > /* do sb->s_aops_lock manipulation */ > > > > Yea that would be ok IMO. > > > > Darrick would just having this be CONFIG_FS_DAX as per this patch be ok with > > you. I guess the static key may have been a bit of overkill? > > > > > > > > 2. global lock > > > - OMG! > > > > I know. The thinking on this is that the locking is percpu which is near > > 0 overhead in the read case and we are rarely going to take exclusive access. > > The problem is that users can effectively run: > > $ xfs_io -c "chattr -R -D -x" /path/to/dax_fs > > And then it walks over millions of individual inodes turning off > the DAX flag on each of them. And if each of those inodes takes a > global per-cpu rwsem that blocks all read/write IO and page faults > on everything even for a short time, then this is will have a major > impact on the entire system and users will be very unhappy. > > > > - global lock will cause entire system IO/page fault stalls > > > when someone does recursive/bulk DAX flag modification > > > operations. Per-cpu rwsem Contention on large systems will be > > > utterly awful. > > > > Yea that is probably bad... I certainly did not test the responsiveness of > > this. FWIW if the system only has 1 FS the per-SB lock is not going to be any > > different from the global which was part of our thinking. > > Don't forget that things like /proc, /sys, etc are all filesystems. > Hence the global lock will affect accesses to -everything- in the > system, not just the DAX enabled filesystem. Global locks are -bad-. > > > > - ext4's use case almost never hits the exclusive lock side of the > > > percpu-rwsem - only when changing the journal mode flag on the > > > inode. And it only affects writeback in progress, so it's not > > > going to have massive concurrency on it like a VFS level global > > > lock has. -> Bad model to follow. > > > > I admit I did not research the ext4's journal mode locking that much. > > > > > - per-sb lock is trivial - see #1 - which limits scope to single > > > filesystem > > > > I agree and... well the commit message shows I actually implemented it that > > way at first... :-/ > > > > > - per-inode rwsem would make this problem go away entirely. > > > > But would that be ok for concurrent read performance which is going to be used > > 99.99% of the time? Maybe Darricks comments made me panic a little bit too > > much overhead WRT locking and its potential impact on users not using DAX? > > I know that a rwsem in shared mode can easily handle 2M lock > cycles/s across a 32p machine without contention (concurrent AIO-DIO > read/writes to a single file) so the baseline performance of a rwsem > is likely good enough to start from. > > It's simple, and we can run tests easily enough to find out where it > starts to become a performance limitation. This whole "global percpu > rwsem thing stinks like premature optimisation. Do the simple, > straight forward thing first, then get numbers and analysis the > limitations to determine what the second generation of the > functionality needs to fix. > > IMO, we don't have to solve every scalability problem with the > initial implementation. Let's just make it work first, then worry > about extreme scalability when we have some idea of where those > scalability problems are. > > > > 3. If we can use a global per-cpu rwsem, why can't we just use a > > > per-inode rwsem? > > > > Per-cpu lock was attractive because of its near 0 overhead to take the read > > lock which happens a lot during normal operations. > > > > > - locking context rules are the same > > > - rwsem scales pretty damn well for shared ops > > > > Does it? I'm not sure. > > If you haven't tested it, then we are definitely in the realm of > premature optimisation... > > > > > > - no "global" contention problems > > > - small enough that we can put another rwsem in the inode. > > > > Everything else I agree with... :-D > > > > > > > > 4. "inode_dax_state_up_read" > > > - Eye bleeds. > > > - this is about the aops structure serialisation, not dax. > > > - The name makes no sense in the places that it has been added. > > > > Again it is about more than just the aops. IS_DAX() needs to be protected in > > all the file operations calls as well or we can get races with the logic in > > those calls and a state switch. > > > > > > > > 5. We already have code that does almost exactly what we need: the > > > superblock freezing infrastructure. > > > > I think freeze does a lot more than we need. > > > > > - freezing implements a "hold operations on this superblock until > > > further notice" model we can easily follow. > > > - sb_start_write/sb_end_write provides a simple API model and a > > > clean, clear and concise naming convention we can use, too. > > > > Ok as a model... If going with the per-SB lock. > > Ok, you completely missed my point. You're still looking at this as > a set of "locks" and serialisation. > > Freezing is *not a lock* - it provides a series of "drain points" > where we can transparently block new operations, then wait for all > existing operations to complete so we can make a state change, and > then once that is done we unblock all the waiters.... > > IOWs, the freeze model provides an ordered barrier mechanism, and > that's precisely what we need for aops protection... > > Yes, it may be implemented with locks internally, but that's > implementation detail of the barrier mechanism, not an indication > what functionality it is actually providing. > > > After working through my > > response I'm leaning toward a per-inode lock again. This was the way I did > > this to begin with. > > > > I want feedback before reworking for V4, please. > > IMO, always do the simple thing first. > > Only do a more complex thing if the simple thing doesn't work or > doesn't perform sufficiently well for an initial implemenation. > Otherwise we end up with crazy solutions from premature optimisation > that simply aren't viable. > > > > Really, I'm struggling to understand how we got to "global locking > > > that stops the world" from "need to change per-inode state > > > atomically". Can someone please explain to me why this patch isn't > > > just a simple set of largely self-explanitory functions like this: > > > > > > XX_start_aop() > > > XX_end_aop() > > > > > > XX_lock_aops() > > > XX_switch_aops(aops) > > > XX_unlock_aops() > > > > > > where "XX" is "sb" or "inode" depending on what locking granularity > > > is used... > > > > Because failing to protect the logic around IS_DAX() results in failures in the > > read/write and direct IO paths. > > > > So we need to lock the file operations as well. > > Again, there is no "locking" here. We just need to annotate regions > where the aops vector must remain stable. If a fop calls an aop that > the aops vector does not change while it is the middle of executing > the fop. Hence such fops calls will need to be inside > XX_start_aop()/XX_end_aop() markers. > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
