On Tue, Aug 16, 2022 at 11:58:36AM -0700, Linus Torvalds wrote:> That said, our filldir code is still confusing as hell. And I would > really like to see that "shared vs non-shared" iterator thing go away, > with everybody using the shared one - and filesystems that can't deal > with it using their own lock. > > But that's a completely independent wart in our complicated filldir saga. > > But if somebody were to look at that iterate-vs-iterate_shared, that > would be lovely. A quick grep shows that we don't have *that* many of > the non-shared cases left: > > git grep '\.iterate\>.*=' > > seems to imply that converting them to a "use my own load" wouldn't be > _too_ bad. > > And some of them might actually be perfectly ok with the shared > semantics (ie inode->i_rwsem held just for reading) and they just were > never converted originally.What's depressing is that some of these are newly added. It'd be great if we could attach something _like_ __deprecated to things that checkpatch could pick up on. fs/adfs/dir_f.c: .iterate = adfs_f_iterate, fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate, ADFS is read-only, so must be safe? fs/ceph/dir.c: .iterate = ceph_readdir, fs/ceph/dir.c: .iterate = ceph_readdir, At least CEPH has active maintainers, cc'd fs/coda/dir.c: .iterate = coda_readdir, Would anyone notice if we broke CODA? Maintainers cc'd anyway. fs/exfat/dir.c: .iterate = exfat_iterate, Exfat is a new addition, but has active maintainers. fs/jfs/namei.c: .iterate = jfs_readdir, Maintainer cc'd fs/ntfs/dir.c: .iterate = ntfs_readdir, /* Read directory contents. */ Maybe we can get rid of ntfs soon. fs/ocfs2/file.c: .iterate = ocfs2_readdir, fs/ocfs2/file.c: .iterate = ocfs2_readdir, maintainers cc'd fs/orangefs/dir.c: .iterate = orangefs_dir_iterate, New; maintainer cc'd fs/overlayfs/readdir.c: .iterate = ovl_iterate, Active maintainer, cc'd fs/proc/base.c: .iterate = proc_##LSM##_attr_dir_iterate, \ Hmm. We need both SMACK and Apparmor to agree to this ... cc's added. fs/vboxsf/dir.c: .iterate = vboxsf_dir_iterate, Also newly added. Maintainer cc'd.
On 8/16/2022 12:11 PM, Matthew Wilcox wrote:> On Tue, Aug 16, 2022 at 11:58:36AM -0700, Linus Torvalds wrote: >> That said, our filldir code is still confusing as hell. And I would >> really like to see that "shared vs non-shared" iterator thing go away, >> with everybody using the shared one - and filesystems that can't deal >> with it using their own lock. >> >> But that's a completely independent wart in our complicated filldir saga. >> >> But if somebody were to look at that iterate-vs-iterate_shared, that >> would be lovely. A quick grep shows that we don't have *that* many of >> the non-shared cases left: >> >> git grep '\.iterate\>.*=' >> >> seems to imply that converting them to a "use my own load" wouldn't be >> _too_ bad. >> >> And some of them might actually be perfectly ok with the shared >> semantics (ie inode->i_rwsem held just for reading) and they just were >> never converted originally. > What's depressing is that some of these are newly added. It'd be > great if we could attach something _like_ __deprecated to things > that checkpatch could pick up on. > > fs/adfs/dir_f.c: .iterate = adfs_f_iterate, > fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate, > > ADFS is read-only, so must be safe? > > fs/ceph/dir.c: .iterate = ceph_readdir, > fs/ceph/dir.c: .iterate = ceph_readdir, > > At least CEPH has active maintainers, cc'd > > fs/coda/dir.c: .iterate = coda_readdir, > > Would anyone notice if we broke CODA? Maintainers cc'd anyway. > > fs/exfat/dir.c: .iterate = exfat_iterate, > > Exfat is a new addition, but has active maintainers. > > fs/jfs/namei.c: .iterate = jfs_readdir, > > Maintainer cc'd > > fs/ntfs/dir.c: .iterate = ntfs_readdir, /* Read directory contents. */ > > Maybe we can get rid of ntfs soon. > > fs/ocfs2/file.c: .iterate = ocfs2_readdir, > fs/ocfs2/file.c: .iterate = ocfs2_readdir, > > maintainers cc'd > > fs/orangefs/dir.c: .iterate = orangefs_dir_iterate, > > New; maintainer cc'd > > fs/overlayfs/readdir.c: .iterate = ovl_iterate, > > Active maintainer, cc'd > > fs/proc/base.c: .iterate = proc_##LSM##_attr_dir_iterate, \ > > Hmm. We need both SMACK and Apparmor to agree to this ... cc's added.Smack passes all tests and seems perfectly content with the change. I can't say that the tests stress this interface.> > fs/vboxsf/dir.c: .iterate = vboxsf_dir_iterate, > > Also newly added. Maintainer cc'd.
On Tue, Aug 16, 2022 at 08:11:31PM +0100, Matthew Wilcox wrote:> fs/adfs/dir_f.c: .iterate = adfs_f_iterate, > fs/adfs/dir_fplus.c: .iterate = adfs_fplus_iterate, > > ADFS is read-only, so must be safe?I just checked ADFS. This isn't a f_ops ->iterate, this is a special adfs_dir_ops. ADFS already uses f_ops->iterate_shared.