On Mon, Jan 4, 2016 at 7:51 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > On Mon, Jan 04, 2016 at 10:20:05AM -0800, Dan Williams wrote: >> Historically we have waited for filesystem specific heuristics to >> attempt to guess when a block device is gone. Sometimes this works, but >> in other cases the system can hang waiting for the fs to trigger its >> shutdown protocol. >> >> The initial motivation for this investigation was to prevent DAX >> mappings (direct mmap access to persistent memory) from leaking past the >> lifetime of the hosting block device. However, Dave points out that >> these shutdown operations are needed in other scenarios. Quoting Dave: >> >> For example, if we detect a free space corruption during allocation, >> it is not safe to trust *any active mapping* because we can't trust >> that we having handed out the same block to multiple owners. Hence >> on such a filesystem shutdown, we have to prevent any new DAX >> mapping from occurring and invalidate all existing mappings as we >> cannot allow userspace to modify any data or metadata until we've >> resolved the corruption situation. >> >> The current block device shutdown sequence of del_gendisk + >> blk_cleanup_queue is problematic. We want to tell the fs after >> blk_cleanup_queue that there is no possibility of recovery, but by that >> time we have deleted partitions and lost the ability to find all the >> super-blocks on a block device. >> >> Introduce del_gendisk_queue to trigger ->quiesce() and ->bdi_gone() > > I don't see anything that introduces a ->quiesce() method. > Right, we only have generic_quiesce_super() as no fs had a need to do anything but the generic actions. >> notifications to all the filesystems hosted on the disk. Where >> ->quiesce() are 'shutdown' operations while the bdev may still be alive, > > So you are saying "quiesce == invalidation", which is in conflict > with what we typically think quiesce means. i.e. "Quiesce" is what > we do when doing orderly writeback of all outstanding dirty objects > in a filesystem - what we do during freeze, remount-ro, and unmount. > e.g. See the functions xfs_log_quiesce() and xfs_attr_quiesce() Fair enough, I should have called this one invalidate_super. > >> and ->bdi_gone() is a set of actions to take after the backing device >> is known to be permanently dead. > > In which case, bdi_gone == shutdown. > True, and following this logic I think the existing generic_shutdown_super() should be renamed generic_kill_super() to match the fs actions, but see below... > Operation methods should be named after what they do, not what their > calling context is. i.e. these are "invalidate" and "shutdown" > superblock operations, not "quiesce" and "bdi_gone". I was running out of colors to paint the bike shed given generic_shutdown_super() was already in use. Also, since generic_shutdown_super() has had its current meaning since forever I didn't think it was worth the risk to change the meaning of such a long standing symbol. Other ideas, generic_stop_super? >> generic_quiesce_super and generic_bdi_gone, are the default operations >> when a filesystem does not implement ->quiesce(), ->bdi_gone(). They >> invalidate inodes and unmap DAX-inodes respectively. For now only >> ->bdi_gone() has an associated super operation as xfs will implement >> ->bdi_gone() in a later patch. > > I don't quite understand what the point of factoring > __invalidate_device() like this is - it's not used by anyone, so it > seems completely unnecessary to me. > You're right, without a need for an fs to intercept the 'invalidation' event there's no need to do this refactor. > And really, that points out that there are multiple changes in this > patch set that should be done separately. The rework of > del_gendisk() into del_gendisk_start/del_gendisk_end should be the > first patch. The del_gendisk/blk_cleanup_queue to > blk_cleanup_queue() combining should be the second part, as it > builds on the factoring of del_gendisk(). Then, if we really need to > keep it, the factoring to introduce generic_quiesce_super. And > finally, the patch that allows the shutdown callout can be > introduced. Ok, will split. > > [snip] >> >> +static void generic_bdi_gone(struct super_block *sb) >> +{ >> + struct inode *inode, *_inode = NULL; >> + >> + spin_lock(&sb->s_inode_list_lock); >> + list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { >> + spin_lock(&inode->i_lock); >> + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) >> + || !IS_DAX(inode)) { >> + spin_unlock(&inode->i_lock); >> + continue; >> + } >> + __iget(inode); >> + spin_unlock(&inode->i_lock); >> + spin_unlock(&sb->s_inode_list_lock); >> + >> + unmap_mapping_range(inode->i_mapping, 0, 0, 1); >> + iput(_inode); >> + _inode = inode; >> + cond_resched(); >> + >> + spin_lock(&sb->s_inode_list_lock); >> + } >> + spin_unlock(&sb->s_inode_list_lock); >> + iput(_inode); >> +} > > This belongs in fs/inode.c, right next to invalidate_inodes(), and > with a name that describes what it does, not the context in which > it is called. e.g. unmap_dax_inodes(). > Sounds good. >> +void shutdown_partition(struct gendisk *disk, int partno) >> +{ >> + struct block_device *bdev = bdget_disk(disk, partno); >> + struct super_block *sb = get_super(bdev); >> + >> + if (!bdev) >> + return; > > Null pointer deref. Certainly a landmine waiting for someone to > tread on. > Nope, get_super() checks for a NULL argument. >> + if (!sb) { >> + bdput(bdev); >> + return; >> + } >> + >> + if (sb->s_op->bdi_gone) >> + sb->s_op->bdi_gone(sb); >> + else >> + generic_bdi_gone(sb); > > if (sb->s_op->shutdown) > sb->s_op->shutdown(sb); > else > unmap_dax_inodes(sb); > > name things correctly, and the code documents itself. How about 'stop' or 'halt' instead of 'shutdown' to preserve the historical meaning of generic_shutdown_super? _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs