| Message ID | 20210729134943.778917-1-almaz.alexandrovich@paragon-software.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | NTFS read-write driver GPL implementation by Paragon Software | expand |
On Thu, Jul 29, 2021 at 04:49:33PM +0300, Konstantin Komarov wrote: > This patch adds NTFS Read-Write driver to fs/ntfs3. > > Having decades of expertise in commercial file systems development and huge > test coverage, we at Paragon Software GmbH want to make our contribution to > the Open Source Community by providing implementation of NTFS Read-Write > driver for the Linux Kernel. > > This is fully functional NTFS Read-Write driver. Current version works with > NTFS(including v3.1) and normal/compressed/sparse files and supports journal replaying. > > We plan to support this version after the codebase once merged, and add new > features and fix bugs. For example, full journaling support over JBD will be > added in later updates. Cool! I have the same (still unanswered) questions as last time: 1. What happens when you run ntfs3 through fstests with '-g all'? I get that the pass rate isn't going to be as high with ntfs3 as it is with ext4/xfs/btrfs, but fstests can be adapted (see the recent attempts to get exfat under test). (And yes, not all the fstests will pass, since some of them are random exercisers that we use to keep the bug backlog populated. We can help you figure out which tests are erratic like that). 2. Same question as #1, except for whatever internal QA suite Paragon has used to qualify the ntfs3 driver over the years. (If you haven't been using fstests this whole time.) 3. If you (Paragon) do have an internal QA suite, are you willing to contribute some of that to fstests to improve its ability to exercise whatever features and quirks are unique to NTFS? 4. How often do you run QA validation (of any kind) on the ntfs3 codebase? In case you're wondering why I ask these questions, my motivation is in figuring out how easy it will be to extend QA coverage to the community supported QA suite (fstests) so that people making treewide and vfs level changes can check that their changes don't bitrot your driver, and vice-versa. My primary interest leans towards convincing everyone to value QA and practice it regularly (aka sharing the load so it's not entirely up to the maintainer to catch all problems) vs. finding every coding error as a gate condition for merging. As another fs maintainer, I know that this is key to preventing fs drivers from turning into a rotting garbage fire, and probably the best I can do for a review of ntfs3 since I don't anticipate having time for a super-detailed review and you've been submitting this driver for a while now. --D > > v2: > - patch splitted to chunks (file-wise) > - build issues fixed > - sparse and checkpatch.pl errors fixed > - NULL pointer dereference on mkfs.ntfs-formatted volume mount fixed > - cosmetics + code cleanup > > v3: > - added acl, noatime, no_acs_rules, prealloc mount options > - added fiemap support > - fixed encodings support > - removed typedefs > - adapted Kernel-way logging mechanisms > - fixed typos and corner-case issues > > v4: > - atomic_open() refactored > - code style updated > - bugfixes > > v5: > - nls/nls_alt mount options added > - Unicode conversion fixes > - Improved very fragmented files operations > - logging cosmetics > > v6: > - Security Descriptors processing changed > added system.ntfs_security xattr to set > SD > - atomic_open() optimized > - cosmetics > > v7: > - Security Descriptors validity checks added (by Mark Harmstone) > - atomic_open() fixed for the compressed file creation with directio > case > - remount support > - temporarily removed readahead usage > - cosmetics > > v8: > - Compressed files operations fixed > > v9: > - Further cosmetics applied as suggested > by Joe Perches > > v10: > - operations with compressed/sparse files on very fragmented volumes improved > - reduced memory consumption for above cases > > v11: > - further compressed files optimizations: reads/writes are now skipping bufferization > - journal wipe to the initial state optimized (bufferization is also skipped) > - optimized run storage (re-packing cluster metainformation) > - fixes based on Matthew Wilcox feedback to the v10 > - compressed/sparse/normal could be set for empty files with 'system.ntfs_attrib' xattr > > v12: > - nls_alt mount option removed after discussion with Pali Rohar > - fixed ni_repack() > - fixed resident files transition to non-resident when size increasing > > v13: > - nested_lock fix (lockdep) > - out-of-bounds read fix (KASAN warning) > - resident->nonresident transition fixed for compressed files > - load_nls() missed fix applied > - some sparse utility warnings fixes > > v14: > - support for additional compression types (we've adapted WIMLIB's > implementation, authored by Eric Biggers, into ntfs3) > > v15: > - kernel test robot warnings fixed > - lzx/xpress compression license headers updated > > v16: > - lzx/xpress moved to initial ntfs-3g plugin code > - mutexes instead of a global spinlock for compresions > - FALLOC_FL_PUNCH_HOLE and FALLOC_FL_COLLAPSE_RANGE implemented > - CONFIG_NTFS3_FS_POSIX_ACL added > > v17: > - FALLOC_FL_COLLAPSE_RANGE fixed > - fixes for Mattew Wilcox's and Andy Lavr's concerns > > v18: > - ntfs_alloc macro splitted into two ntfs_malloc + ntfs_zalloc > - attrlist.c: always use ntfs_cmp_names instead of memcmp; compare entry names > only for entry with vcn == 0 > - dir.c: remove unconditional ni_lock in ntfs_readdir > - fslog.c: corrected error case behavior > - index.c: refactored due to modification of ntfs_cmp_names; use rw_semaphore > for read/write access to alloc_run and bitmap_run while ntfs_readdir > - run.c: separated big/little endian code in functions > - upcase.c: improved ntfs_cmp_names, thanks to Kari Argillander for idea > and 'bothcase' implementation > > v19: > - fixed directory bitmap for 2MB cluster size > - fixed rw_semaphore init for directories > > v20: > - fixed issue with incorrect hidden/system attribute setting on > root subdirectories > - use kvmalloc instead of kmalloc for runs array > - fixed index behavior on volumes with cluster size more than 4k > - current build info is added into module info instead of printing on insmod > > v21: > - fixes for clang CFI checks > - fixed sb->s_maxbytes for 32bit clusters > - user.DOSATTRIB is no more intercepted by ntfs3 > - corrected xattr limits; is used > - corrected CONFIG_NTFS3_64BIT_CLUSTER usage > - info about current build is added into module info and printing > on insmod (by Andy Lavr's request) > note: v21 is applicable for 'linux-next' not older than 2021.01.28 > > v22: > - ntfs_cmp_names() fixed > - raise warning if 'nls->uni2char' fails > - hot fix free clusters code optimized > - use clang-format 11.0 instead of 10.0 to format code > > v23: > - corrections for Kernel Test Robot warnings > - kmem_cache_create() utilized to allocate memory in bitmap.c > - cosmetics and comments thru the code > > v24: > - BIO_MAX_PAGES -> BIO_MAX_VECS (fix for build issue of v23 vs linux-next) > - minor optimization for LogFile: do not fill it with -1, if it's already there > - index.c: removed 'inline' in definition of hdr_find_split() and hdr_insert_head() > > v25: > - restore fs/Makefile in patch > - refactor ntfs_create_inode() to use error-valued pointer > - use mi_get_ref to fill MFT_REF > - minimize checkpatch.pl warnings: replace LogFile with \x24LogFile when printing > > v26: > - fixed coccinelle warnings > - fslog.c: fix memory leak and memory overwrite > > v27: > - iov_iter_copy_from_user_atomic() replaced by copy_page_from_iter_atomic() > and iov_iter_advance() removed > > Konstantin Komarov (10): > fs/ntfs3: Add headers and misc files > fs/ntfs3: Add initialization of super block > fs/ntfs3: Add bitmap > fs/ntfs3: Add file operations and implementation > fs/ntfs3: Add attrib operations > fs/ntfs3: Add compression > fs/ntfs3: Add NTFS journal > fs/ntfs3: Add Kconfig, Makefile and doc > fs/ntfs3: Add NTFS3 in fs/Kconfig and fs/Makefile > fs/ntfs3: Add MAINTAINERS > > Documentation/filesystems/ntfs3.rst | 107 + > MAINTAINERS | 7 + > fs/Kconfig | 1 + > fs/Makefile | 1 + > fs/ntfs3/Kconfig | 46 + > fs/ntfs3/Makefile | 36 + > fs/ntfs3/attrib.c | 2082 +++++++++++ > fs/ntfs3/attrlist.c | 456 +++ > fs/ntfs3/bitfunc.c | 135 + > fs/ntfs3/bitmap.c | 1519 ++++++++ > fs/ntfs3/debug.h | 64 + > fs/ntfs3/dir.c | 594 +++ > fs/ntfs3/file.c | 1130 ++++++ > fs/ntfs3/frecord.c | 3071 ++++++++++++++++ > fs/ntfs3/fslog.c | 5181 +++++++++++++++++++++++++++ > fs/ntfs3/fsntfs.c | 2542 +++++++++++++ > fs/ntfs3/index.c | 2641 ++++++++++++++ > fs/ntfs3/inode.c | 2034 +++++++++++ > fs/ntfs3/lib/decompress_common.c | 332 ++ > fs/ntfs3/lib/decompress_common.h | 352 ++ > fs/ntfs3/lib/lib.h | 26 + > fs/ntfs3/lib/lzx_decompress.c | 683 ++++ > fs/ntfs3/lib/xpress_decompress.c | 155 + > fs/ntfs3/lznt.c | 452 +++ > fs/ntfs3/namei.c | 578 +++ > fs/ntfs3/ntfs.h | 1238 +++++++ > fs/ntfs3/ntfs_fs.h | 1085 ++++++ > fs/ntfs3/record.c | 609 ++++ > fs/ntfs3/run.c | 1111 ++++++ > fs/ntfs3/super.c | 1500 ++++++++ > fs/ntfs3/upcase.c | 105 + > fs/ntfs3/xattr.c | 1046 ++++++ > 32 files changed, 30919 insertions(+) > create mode 100644 Documentation/filesystems/ntfs3.rst > create mode 100644 fs/ntfs3/Kconfig > create mode 100644 fs/ntfs3/Makefile > create mode 100644 fs/ntfs3/attrib.c > create mode 100644 fs/ntfs3/attrlist.c > create mode 100644 fs/ntfs3/bitfunc.c > create mode 100644 fs/ntfs3/bitmap.c > create mode 100644 fs/ntfs3/debug.h > create mode 100644 fs/ntfs3/dir.c > create mode 100644 fs/ntfs3/file.c > create mode 100644 fs/ntfs3/frecord.c > create mode 100644 fs/ntfs3/fslog.c > create mode 100644 fs/ntfs3/fsntfs.c > create mode 100644 fs/ntfs3/index.c > create mode 100644 fs/ntfs3/inode.c > create mode 100644 fs/ntfs3/lib/decompress_common.c > create mode 100644 fs/ntfs3/lib/decompress_common.h > create mode 100644 fs/ntfs3/lib/lib.h > create mode 100644 fs/ntfs3/lib/lzx_decompress.c > create mode 100644 fs/ntfs3/lib/xpress_decompress.c > create mode 100644 fs/ntfs3/lznt.c > create mode 100644 fs/ntfs3/namei.c > create mode 100644 fs/ntfs3/ntfs.h > create mode 100644 fs/ntfs3/ntfs_fs.h > create mode 100644 fs/ntfs3/record.c > create mode 100644 fs/ntfs3/run.c > create mode 100644 fs/ntfs3/super.c > create mode 100644 fs/ntfs3/upcase.c > create mode 100644 fs/ntfs3/xattr.c > > > base-commit: 5a4cee98ea757e1a2a1354b497afdf8fafc30a20 > -- > 2.25.4 >
Le 29/07/2021 à 15:49, Konstantin Komarov a écrit : > This adds file operations and implementation > > Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > --- > fs/ntfs3/dir.c | 594 +++++++++ > fs/ntfs3/file.c | 1130 ++++++++++++++++ > fs/ntfs3/frecord.c | 3071 ++++++++++++++++++++++++++++++++++++++++++++ > fs/ntfs3/namei.c | 578 +++++++++ > fs/ntfs3/record.c | 609 +++++++++ > fs/ntfs3/run.c | 1111 ++++++++++++++++ > 6 files changed, 7093 insertions(+) > create mode 100644 fs/ntfs3/dir.c > create mode 100644 fs/ntfs3/file.c > create mode 100644 fs/ntfs3/frecord.c > create mode 100644 fs/ntfs3/namei.c > create mode 100644 fs/ntfs3/record.c > create mode 100644 fs/ntfs3/run.c > [...] > diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c > new file mode 100644 > index 000000000..f5db12cd3 > --- /dev/null > +++ b/fs/ntfs3/namei.c [...] > +/* > + * ntfs_rename > + * > + * inode_operations::rename > + */ > +static int ntfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > + struct dentry *old_dentry, struct inode *new_dir, > + struct dentry *new_dentry, u32 flags) > +{ > + int err; > + struct super_block *sb = old_dir->i_sb; > + struct ntfs_sb_info *sbi = sb->s_fs_info; > + struct ntfs_inode *old_dir_ni = ntfs_i(old_dir); > + struct ntfs_inode *new_dir_ni = ntfs_i(new_dir); > + struct ntfs_inode *old_ni; > + struct ATTR_FILE_NAME *old_name, *new_name, *fname; > + u8 name_type; > + bool is_same; > + struct inode *old_inode, *new_inode; > + struct NTFS_DE *old_de, *new_de; > + struct ATTRIB *attr; > + struct ATTR_LIST_ENTRY *le; > + u16 new_de_key_size; > + > + static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + SIZEOF_RESIDENT < 1024); > + static_assert(SIZEOF_ATTRIBUTE_FILENAME_MAX + sizeof(struct NTFS_DE) < > + 1024); > + static_assert(PATH_MAX >= 4 * 1024); > + > + if (flags & ~RENAME_NOREPLACE) > + return -EINVAL; > + > + old_inode = d_inode(old_dentry); > + new_inode = d_inode(new_dentry); > + > + old_ni = ntfs_i(old_inode); > + > + is_same = old_dentry->d_name.len == new_dentry->d_name.len && > + !memcmp(old_dentry->d_name.name, new_dentry->d_name.name, > + old_dentry->d_name.len); > + > + if (is_same && old_dir == new_dir) { > + /* Nothing to do */ > + err = 0; > + goto out; > + } > + > + if (ntfs_is_meta_file(sbi, old_inode->i_ino)) { > + err = -EINVAL; > + goto out; > + } > + > + if (new_inode) { > + /*target name exists. unlink it*/ > + dget(new_dentry); > + ni_lock_dir(new_dir_ni); > + err = ntfs_unlink_inode(new_dir, new_dentry); > + ni_unlock(new_dir_ni); > + dput(new_dentry); > + if (err) > + goto out; > + } > + > + /* allocate PATH_MAX bytes */ > + old_de = __getname(); > + if (!old_de) { > + err = -ENOMEM; > + goto out; > + } > + > + err = fill_name_de(sbi, old_de, &old_dentry->d_name, NULL); > + if (err < 0) > + goto out1; > + > + old_name = (struct ATTR_FILE_NAME *)(old_de + 1); > + > + if (is_same) { > + new_de = old_de; > + } else { > + new_de = Add2Ptr(old_de, 1024); > + err = fill_name_de(sbi, new_de, &new_dentry->d_name, NULL); > + if (err < 0) > + goto out1; > + } > + > + ni_lock_dir(old_dir_ni); > + ni_lock(old_ni); > + > + mi_get_ref(&old_dir_ni->mi, &old_name->home); > + > + /*get pointer to file_name in mft*/ > + fname = ni_fname_name(old_ni, (struct cpu_str *)&old_name->name_len, > + &old_name->home, &le); > + if (!fname) { > + err = -EINVAL; > + goto out2; > + } > + > + /* Copy fname info from record into new fname */ > + new_name = (struct ATTR_FILE_NAME *)(new_de + 1); > + memcpy(&new_name->dup, &fname->dup, sizeof(fname->dup)); > + > + name_type = paired_name(fname->type); > + > + /* remove first name from directory */ > + err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, > + le16_to_cpu(old_de->key_size), sbi); > + if (err) > + goto out3; > + > + /* remove first name from mft */ > + err = ni_remove_attr_le(old_ni, attr_from_name(fname), le); > + if (err) > + goto out4; > + > + le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); > + old_ni->mi.dirty = true; > + > + if (name_type != FILE_NAME_POSIX) { > + /* get paired name */ > + fname = ni_fname_type(old_ni, name_type, &le); > + if (fname) { > + /* remove second name from directory */ > + err = indx_delete_entry(&old_dir_ni->dir, old_dir_ni, > + fname, fname_full_size(fname), > + sbi); > + if (err) > + goto out5; > + > + /* remove second name from mft */ > + err = ni_remove_attr_le(old_ni, attr_from_name(fname), > + le); > + if (err) > + goto out6; > + > + le16_add_cpu(&old_ni->mi.mrec->hard_links, -1); > + old_ni->mi.dirty = true; > + } > + } > + > + /* Add new name */ > + mi_get_ref(&old_ni->mi, &new_de->ref); > + mi_get_ref(&ntfs_i(new_dir)->mi, &new_name->home); > + > + new_de_key_size = le16_to_cpu(new_de->key_size); > + > + /* insert new name in mft */ > + err = ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, > + &attr, NULL); > + if (err) > + goto out7; > + > + attr->res.flags = RESIDENT_FLAG_INDEXED; > + > + memcpy(Add2Ptr(attr, SIZEOF_RESIDENT), new_name, new_de_key_size); > + > + le16_add_cpu(&old_ni->mi.mrec->hard_links, 1); > + old_ni->mi.dirty = true; > + > + /* insert new name in directory */ > + err = indx_insert_entry(&new_dir_ni->dir, new_dir_ni, new_de, sbi, > + NULL); > + if (err) > + goto out8; > + > + if (IS_DIRSYNC(new_dir)) > + err = ntfs_sync_inode(old_inode); This value returned by 'ntfs_sync_inode()' is silenced below. Maybe the same should be done here? Anyway, this 'err' is never used and is forced to 0 below > + else > + mark_inode_dirty(old_inode); > + > + old_dir->i_ctime = old_dir->i_mtime = current_time(old_dir); > + if (IS_DIRSYNC(old_dir)) > + (void)ntfs_sync_inode(old_dir); here > + else > + mark_inode_dirty(old_dir); > + > + if (old_dir != new_dir) { > + new_dir->i_mtime = new_dir->i_ctime = old_dir->i_ctime; > + mark_inode_dirty(new_dir); > + } > + > + if (old_inode) { > + old_inode->i_ctime = old_dir->i_ctime; > + mark_inode_dirty(old_inode); > + } > + > + err = 0; and here. > + /* normal way */ > + goto out2; > + > +out8: > + /* undo > + * ni_insert_resident(old_ni, new_de_key_size, ATTR_NAME, NULL, 0, > + * &attr, NULL); > + */ > + mi_remove_attr(&old_ni->mi, attr); > +out7: > + /* undo > + * ni_remove_attr_le(old_ni, attr_from_name(fname), le); > + */ > +out6: > + /* undo > + * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, > + * fname, fname_full_size(fname), > + * sbi); > + */ > +out5: > + /* undo > + * ni_remove_attr_le(old_ni, attr_from_name(fname), le); > + */ > +out4: > + /* undo: > + * indx_delete_entry(&old_dir_ni->dir, old_dir_ni, old_de + 1, > + * old_de->key_size, NULL); > + */ > +out3: > +out2: > + ni_unlock(old_ni); > + ni_unlock(old_dir_ni); > +out1: > + __putname(old_de); > +out: > + return err; > +} [...]
On Thu, Jul 29, 2021 at 09:24:59AM -0700, Darrick J. Wong wrote: > > I have the same (still unanswered) questions as last time: > > 1. What happens when you run ntfs3 through fstests with '-g all'? I get > that the pass rate isn't going to be as high with ntfs3 as it is with > ext4/xfs/btrfs, but fstests can be adapted (see the recent attempts to > get exfat under test). Indeed, it's not that hard at all. I've included a patch to xfstests-bld[1] so that you can just run "kvm-xfstests -c ntfs3 -g auto". Konstantin, I would *strongly* encourage you to try running fstests, about 60 seconds into a run, we discover that generic/013 will trigger locking problems that could lead to deadlocks. The test generic/091 will also trigger kernel NULL dereference BUG: BUG: kernel NULL pointer dereference, address: 0000000000000008 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] SMP NOPTI CPU: 0 PID: 23029 Comm: fsx Not tainted 5.14.0-rc2-xfstests-00010-gdf9570 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/4 RIP: 0010:__bio_add_page+0x46/0x80 Code: 39 47 5a 76 3d 41 89 d0 41 f7 d0 44 39 47 28 77 31 48 89 30 89 48 5 RSP: 0018:ffffa3fa05e13900 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000001000 RCX: 0000000000000000 RDX: 0000000000001000 RSI: 0000000000000000 RDI: ffff908b8f930b40 RBP: ffff908b8f930b40 R08: 00000000ffffefff R09: 0000000000000000 R10: ffffffffa4973270 R11: 0000000000000002 R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 FS: 00007f04a8ba2740(0000) GS:ffff908bfda00000(0000) knlGS:0000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000008 CR3: 0000000010984005 CR4: 0000000000770ef0 PKRU: 55555554 Call Trace: bio_add_page+0x62/0x90 submit_bh_wbc+0xe3/0x190 ll_rw_block+0xaa/0xb0 ntfs_get_block_vbo+0x305/0x430 do_direct_IO+0x3af/0xa20 do_blockdev_direct_IO+0x2b7/0x960 ? ntfs_get_block_write_begin+0x20/0x20 ? ntfs_get_block_write_begin+0x20/0x20 ? end_buffer_read_nobh+0x30/0x30 ntfs_direct_IO+0xe5/0x1f0 ? touch_atime+0x36/0x250 generic_file_read_iter+0x8c/0x170 generic_file_splice_read+0xfc/0x1b0 splice_direct_to_actor+0xc3/0x230 ? do_splice_to+0xc0/0xc0 do_splice_direct+0x91/0xd0 vfs_copy_file_range+0x144/0x450 __do_sys_copy_file_range+0xc1/0x200 do_syscall_64+0x38/0x90 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7f04a8c98f59 Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 8 RSP: 002b:00007ffc8a2202b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000146 RAX: ffffffffffffffda RBX: 000000000000d000 RCX: 00007f04a8c98f59 RDX: 0000000000000003 RSI: 00007ffc8a220300 RDI: 0000000000000003 RBP: 0000000000000000 R08: 000000000000d000 R09: 0000000000000000 R10: 00007ffc8a220308 R11: 0000000000000246 R12: 00007ffc8a220300 R13: 00007ffc8a220308 R14: 000000000000d000 R15: 000000000004c000 CR2: 0000000000000008 ---[ end trace 1412a19831693976 ]--- It should also be noted that there doesn't appear to be an fsck for ntfs --- there is ntfsfix in the ntfs-3g package but it's a full replacement for chkdsk, and it will often throw up its hands, and tell you to boot into Windows and run CHKDSK. To be fair, this is also true for the fuse-based ntfs implementation --- but at least the fuse-based ntfs implementation won't deadlock your kernel or OOPS the kernel with null pointer dereferences.... :-) > > In case you're wondering why I ask these questions, my motivation is > in figuring out how easy it will be to extend QA coverage to the > community supported QA suite (fstests) so that people making treewide > and vfs level changes can check that their changes don't bitrot your > driver, and vice-versa. My primary interest leans towards convincing > everyone to value QA and practice it regularly (aka sharing the load so > it's not entirely up to the maintainer to catch all problems) vs. > finding every coding error as a gate condition for merging. There are some changes that I've identified to make fstests support fstests more cleanly. But the real problem is the very weak level of userspace tools available for NTFS today. Is this something that Paragon Software is planning on remedying? - Ted Note: this isn't ready for prime-time yet, since the way I've hacked up /sbin/mkfs.ntfs3 causes gce-xfstests to fail. So this isn't going to be going to xfsteests-bld upstream just yet. But it's good enough to make kvm-xfstests work.... Support for the fuse-based ntfs is also not complete for kvm-xfstests, although the config files for ntfs didn't take that much time to do at the same time. Support for the fuse-based ntfs will require some changes to _fs_type in common/rc so that fstests won't barf because when you mount -t ntfs, /proc/mounts and df -T show a file system type of "fuseblk". What I've done so far was just a quick hack, because I was curious how ready for prime-time ntfs3 might currently be. :-) But it does demonstrate how easy it is to run fstests on ntfs3. The hard part will be fixing all of the bugs that it uncovers --- but better that you discover them now, rather than your customers (not to mention the customers for all of us who work for various Linux distributions and hyperscale cloud companies!). commit c2899d9c0251078d9088b44cc7c583c192edd8a4 Author: Theodore Ts'o <tytso@mit.edu> Date: Sun Aug 1 20:47:35 2021 -0400 test-appliance: add support for ntfs and ntfs3 file systems Signed-off-by: Theodore Ts'o <tytso@mit.edu> diff --git a/kvm-xfstests/test-appliance/files/root/fs/ntfs/cfg/all.list b/kvm-xfstests/test-appliance/files/root/fs/ntfs/cfg/all.list new file mode 100644 index 0000000..4ad96d5 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/root/fs/ntfs/cfg/all.list @@ -0,0 +1 @@ +default diff --git a/kvm-xfstests/test-appliance/files/root/fs/ntfs/cfg/default b/kvm-xfstests/test-appliance/files/root/fs/ntfs/cfg/default new file mode 100644 index 0000000..8280c69 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/root/fs/ntfs/cfg/default @@ -0,0 +1,4 @@ +SIZE=small +export MKFS_OPTIONS="" +export NTFS_MOUNT_OPTIONS="" +TESTNAME="ntfs" diff --git a/kvm-xfstests/test-appliance/files/root/fs/ntfs/config b/kvm-xfstests/test-appliance/files/root/fs/ntfs/config new file mode 100644 index 0000000..bee23a5 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/root/fs/ntfs/config @@ -0,0 +1,64 @@ +# +# Configuration file for ntfs +# + +DEFAULT_MKFS_OPTIONS="" + +function check_filesystem() +{ + local dev="$1" + local ret + + /bin/ntfsfix "$dev" + ret="$?" + echo ntfsfix exited with status "$ret" + return "$ret" +} + +function format_filesystem() +{ + local dev="$1" + local opts="$2" + local ret + + /sbin/mkfs.ntfs -f $opts "$dev" + ret="$?" + return "$ret" +} + +function setup_mount_opts() +{ + if test -n "$MNTOPTS" ; then + if test -n "$NTFS_MOUNT_OPTIONS" ; then + export NTFS_MOUNT_OPTIONS="$MOUNT_OPTIONS,$MNTOPTS" + else + export NTFS_MOUNT_OPTIONS="-o $MNTOPTS" + fi + fi +} + +function get_mkfs_opts() +{ + echo "$NTFS_MKFS_OPTIONS" +} + +function show_mkfs_opts() +{ + echo NTFS_MKFS_OPTIONS: "$NTFS_MKFS_OPTIONS" +} + +function show_mount_opts() +{ + echo NTFS_MOUNT_OPTIONS: "NTFS_MOUNT_OPTIONS" +} + +function test_name_alias() +{ + echo "$1" +} + +function reset_vars() +{ + unset NTFS_MOUNT_OPTIONS + unset MKFS_OPTIONS +} diff --git a/kvm-xfstests/test-appliance/files/root/fs/ntfs3/cfg/all.list b/kvm-xfstests/test-appliance/files/root/fs/ntfs3/cfg/all.list new file mode 100644 index 0000000..4ad96d5 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/root/fs/ntfs3/cfg/all.list @@ -0,0 +1 @@ +default diff --git a/kvm-xfstests/test-appliance/files/root/fs/ntfs3/cfg/default b/kvm-xfstests/test-appliance/files/root/fs/ntfs3/cfg/default new file mode 100644 index 0000000..8ba5d07 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/root/fs/ntfs3/cfg/default @@ -0,0 +1,4 @@ +SIZE=small +export MKFS_OPTIONS="" +export NTFS3_MOUNT_OPTIONS="" +TESTNAME="ntfs3" diff --git a/kvm-xfstests/test-appliance/files/root/fs/ntfs3/config b/kvm-xfstests/test-appliance/files/root/fs/ntfs3/config new file mode 100644 index 0000000..6f67e12 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/root/fs/ntfs3/config @@ -0,0 +1,64 @@ +# +# Configuration file for ntfs3 +# + +DEFAULT_MKFS_OPTIONS="" + +function check_filesystem() +{ + local dev="$1" + local ret + + /bin/ntfsfix "$dev" + ret="$?" + echo ntfsfix exited with status "$ret" + return "$ret" +} + +function format_filesystem() +{ + local dev="$1" + local opts="$2" + local ret + + /sbin/mkfs.ntfs -f $opts "$dev" + ret="$?" + return "$ret" +} + +function setup_mount_opts() +{ + if test -n "$MNTOPTS" ; then + if test -n "$NTFS3_MOUNT_OPTIONS" ; then + export NTFS3_MOUNT_OPTIONS="$MOUNT_OPTIONS,$MNTOPTS" + else + export NTFS3_MOUNT_OPTIONS="-o $MNTOPTS" + fi + fi +} + +function get_mkfs_opts() +{ + echo "$NTFS3_MKFS_OPTIONS" +} + +function show_mkfs_opts() +{ + echo NTFS3_MKFS_OPTIONS: "$NTFS3_MKFS_OPTIONS" +} + +function show_mount_opts() +{ + echo NTFS3_MOUNT_OPTIONS: "NTFS3_MOUNT_OPTIONS" +} + +function test_name_alias() +{ + echo "$1" +} + +function reset_vars() +{ + unset NTFS3_MOUNT_OPTIONS + unset MKFS_OPTIONS +} diff --git a/kvm-xfstests/test-appliance/files/sbin/mkfs.ntfs3 b/kvm-xfstests/test-appliance/files/sbin/mkfs.ntfs3 new file mode 100755 index 0000000..f6657a6 --- /dev/null +++ b/kvm-xfstests/test-appliance/files/sbin/mkfs.ntfs3 @@ -0,0 +1,2 @@ +#!/bin/sh +/sbin/mkfs.ntfs -f $* diff --git a/kvm-xfstests/test-appliance/gce-xfstests-bld.sh b/kvm-xfstests/test-appliance/gce-xfstests-bld.sh index befb105..48ce713 100644 --- a/kvm-xfstests/test-appliance/gce-xfstests-bld.sh +++ b/kvm-xfstests/test-appliance/gce-xfstests-bld.sh @@ -73,6 +73,7 @@ PACKAGES="bash-completion \ nbd-server \ nfs-common \ nfs-kernel-server \ + ntfs-3g \ nvme-cli \ openssl \ pciutils \ diff --git a/kvm-xfstests/test-appliance/xfstests-packages b/kvm-xfstests/test-appliance/xfstests-packages index 85ca6a6..6bb8432 100644 --- a/kvm-xfstests/test-appliance/xfstests-packages +++ b/kvm-xfstests/test-appliance/xfstests-packages @@ -33,6 +33,7 @@ mtd-utils multipath-tools nbd-client nbd-server +ntfs-3g nvme-cli parted perl
It should also noted that apparently ntfs3 does not properly support
user namespaces, such that generic/317 fails:
generic/317 [10:37:19][ 7.024574] run fstests generic/317 at 2021-08-02 10:37:19
[10:37:19]- output mismatch (see /results/ntfs3/results-default/generic/317.out.bad)
--- tests/generic/317.out 2021-08-01 20:47:35.000000000 -0400
+++ /results/ntfs3/results-default/generic/317.out.bad 2021-08-02 10:37:19.930687003 -0400
@@ -13,8 +13,8 @@
From init_user_ns
File: "$SCRATCH_MNT/file1"
Size: 0 Filetype: Regular File
- Mode: (0644/-rw-r--r--) Uid: (qa_user) Gid: (qa_user)
+ Mode: (0755/-rwxr-xr-x) Uid: (0) Gid: (0)
From user_ns
File: "$SCRATCH_MNT/file1"
...
(Run 'diff -u /root/xfstests/tests/generic/317.out /results/ntfs3/results-default/generic/317.out.bad' to see the entire diff)
Ran: generic/317
Failures: generic/317
Is Paragon Software willing to commit to fixing these and other bugs?
Better yet, it would be nice if Paragon Software could improve its
testing and other QA processes.
Furthermore, container developers should note that ntfs3 is not
currently safe for use with containers.
- Ted
On Thu, Jul 29, 2021 at 04:49:33PM +0300, Konstantin Komarov wrote: > This patch adds NTFS Read-Write driver to fs/ntfs3. > > Having decades of expertise in commercial file systems development and huge > test coverage, we at Paragon Software GmbH want to make our contribution to > the Open Source Community by providing implementation of NTFS Read-Write > driver for the Linux Kernel. > > This is fully functional NTFS Read-Write driver. Current version works with > NTFS(including v3.1) and normal/compressed/sparse files and supports journal replaying. > > We plan to support this version after the codebase once merged, and add new > features and fix bugs. For example, full journaling support over JBD will be > added in later updates. I'm not expert but I have try to review this series best I can and have not found any major mistakes which prevents merging. Yeah there are couple bugs but because this is not going to replace NTFS driver just yet then IMO it is best that merge will happend sooner so development fot others get easier. I will also try to review future patches (from Paragon and others), test patches and make contribution at my own for this driver. So please use Reviewed by: Kari Argillander <kari.argillander@gmail.com>
On Tue, Aug 10, 2021 at 08:46:37AM +0300, Kari Argillander wrote: > On Thu, Jul 29, 2021 at 04:49:33PM +0300, Konstantin Komarov wrote: > > This patch adds NTFS Read-Write driver to fs/ntfs3. > > > > Having decades of expertise in commercial file systems development and huge > > test coverage, we at Paragon Software GmbH want to make our contribution to > > the Open Source Community by providing implementation of NTFS Read-Write > > driver for the Linux Kernel. > > > > This is fully functional NTFS Read-Write driver. Current version works with > > NTFS(including v3.1) and normal/compressed/sparse files and supports journal replaying. > > > > We plan to support this version after the codebase once merged, and add new > > features and fix bugs. For example, full journaling support over JBD will be > > added in later updates. > > I'm not expert but I have try to review this series best I can and have > not found any major mistakes which prevents merging. Yeah there are > couple bugs but because this is not going to replace NTFS driver just > yet then IMO it is best that merge will happend sooner so development > fot others get easier. I will also try to review future patches (from > Paragon and others), test patches and make contribution at my own for this > driver. So please use > > Reviewed by: Kari Argillander <kari.argillander@gmail.com> Nit: there's supposed to be a dash between 'Reviewed' and 'by'. That said, thanks for putting your name out there! :) --D
On Sun, Aug 01, 2021 at 11:23:16PM -0400, Theodore Ts'o wrote: > On Thu, Jul 29, 2021 at 09:24:59AM -0700, Darrick J. Wong wrote: > > > > I have the same (still unanswered) questions as last time: > > > > 1. What happens when you run ntfs3 through fstests with '-g all'? I get > > that the pass rate isn't going to be as high with ntfs3 as it is with > > ext4/xfs/btrfs, but fstests can be adapted (see the recent attempts to > > get exfat under test). > > Indeed, it's not that hard at all. I've included a patch to > xfstests-bld[1] so that you can just run "kvm-xfstests -c ntfs3 -g > auto". > > Konstantin, I would *strongly* encourage you to try running fstests, > about 60 seconds into a run, we discover that generic/013 will trigger > locking problems that could lead to deadlocks. It seems at least at my testing that if acl option is used then generic/013 will pass. I have tested this with old linux-next commit 5a4cee98ea757e1a2a1354b497afdf8fafc30a20 I have still some of my own code in it but I will test this tomorrow so I can be sure. It also seems that acl support is broken. I also suspect ntfs-3g mkfs in some failure cases. So maybe ntfs-3g mkfs will give different result than Paragons mkfs. It would be nice to test with Paragons mkfs software or that Paragon will test with ntfs-3g.
On Thu, Aug 12, 2021 at 08:03:26PM +0300, Kari Argillander wrote: > On Sun, Aug 01, 2021 at 11:23:16PM -0400, Theodore Ts'o wrote: > > On Thu, Jul 29, 2021 at 09:24:59AM -0700, Darrick J. Wong wrote: > > > > > > I have the same (still unanswered) questions as last time: > > > > > > 1. What happens when you run ntfs3 through fstests with '-g all'? I get > > > that the pass rate isn't going to be as high with ntfs3 as it is with > > > ext4/xfs/btrfs, but fstests can be adapted (see the recent attempts to > > > get exfat under test). > > > > Indeed, it's not that hard at all. I've included a patch to > > xfstests-bld[1] so that you can just run "kvm-xfstests -c ntfs3 -g > > auto". > > > > Konstantin, I would *strongly* encourage you to try running fstests, > > about 60 seconds into a run, we discover that generic/013 will trigger > > locking problems that could lead to deadlocks. > > It seems at least at my testing that if acl option is used then > generic/013 will pass. I have tested this with old linux-next commit > 5a4cee98ea757e1a2a1354b497afdf8fafc30a20 I have still some of my own > code in it but I will test this tomorrow so I can be sure. > > It also seems that acl support is broken. I also suspect ntfs-3g mkfs in > some failure cases. So maybe ntfs-3g mkfs will give different result than > Paragons mkfs. It would be nice to test with Paragons mkfs software or > that Paragon will test with ntfs-3g. I have made more testing and it was actually my code which cause 013 not fail. It is still pretty strange. I have made code for new mount api (fs context) and 013 still get deadlock but still test will pass. This only happends if acl is on. Though this is intresting why this happends. I will not use more time for this now. I will try to focus fs context mount api for now.
> Konstantin, I would *strongly* encourage you to try running fstests, > about 60 seconds into a run, we discover that generic/013 will trigger > locking problems that could lead to deadlocks. Seems like it's a real issue. I was using new HDD and ntfs3-dkms (v26) in Arch and faced locking while was doing rsync: [ 5529.507567] INFO: task kworker/0:1:18 blocked for more than 1105 seconds. [ 5529.507580] Tainted: P OE 5.13.4-arch1-1 #1 [ 5529.507584] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 5529.507586] task:kworker/0:1 state:D stack: 0 pid: 18 ppid: 2 flags:0x00004000 [ 5529.507598] Workqueue: usb_hub_wq hub_event [ 5529.507612] Call Trace: [ 5529.507615] ? out_of_line_wait_on_bit_lock+0xb0/0xb0 [ 5529.507631] __schedule+0x310/0x930 [ 5529.507641] ? out_of_line_wait_on_bit_lock+0xb0/0xb0 [ 5529.507648] schedule+0x5b/0xc0 [ 5529.507654] bit_wait+0xd/0x60 [ 5529.507661] __wait_on_bit+0x2a/0x90 [ 5529.507669] __inode_wait_for_writeback+0xb0/0xe0 [ 5529.507680] ? var_wake_function+0x20/0x20 [ 5529.507689] writeback_single_inode+0x64/0x140 [ 5529.507699] sync_inode_metadata+0x3d/0x60 [ 5529.507712] ntfs_set_state+0x126/0x1a0 [ntfs3] [ 5529.507738] ni_write_inode+0x244/0xef0 [ntfs3] [ 5529.507764] ? pagevec_lookup_range_tag+0x24/0x30 [ 5529.507772] ? __filemap_fdatawait_range+0x6f/0xf0 [ 5529.507785] __writeback_single_inode+0x260/0x310 [ 5529.507795] writeback_single_inode+0xa7/0x140 [ 5529.507803] sync_inode_metadata+0x3d/0x60 [ 5529.507814] ntfs_set_state+0x126/0x1a0 [ntfs3] [ 5529.507834] ntfs_sync_fs+0xf9/0x100 [ntfs3] [ 5529.507857] sync_filesystem+0x40/0x90 [ 5529.507868] fsync_bdev+0x21/0x60 [ 5529.507874] delete_partition+0x13/0x80 [ 5529.507882] blk_drop_partitions+0x5b/0xa0 [ 5529.507889] del_gendisk+0xa5/0x220 [ 5529.507895] sd_remove+0x3d/0x80 [ 5529.507907] __device_release_driver+0x17a/0x230 [ 5529.507918] device_release_driver+0x24/0x30 [ 5529.507927] bus_remove_device+0xdb/0x140 [ 5529.507937] device_del+0x18b/0x400 [ 5529.507943] ? ata_tlink_match+0x30/0x30 [ 5529.507949] ? attribute_container_device_trigger+0xc5/0x100 [ 5529.507959] __scsi_remove_device+0x118/0x150 [ 5529.507967] scsi_forget_host+0x54/0x60 [ 5529.507978] scsi_remove_host+0x72/0x110 [ 5529.507988] usb_stor_disconnect+0x46/0xb0 [usb_storage] [ 5529.508003] usb_unbind_interface+0x8a/0x270 [ 5529.508010] ? kernfs_find_ns+0x35/0xd0 [ 5529.508017] __device_release_driver+0x17a/0x230 [ 5529.508027] device_release_driver+0x24/0x30 [ 5529.508036] bus_remove_device+0xdb/0x140 [ 5529.508044] device_del+0x18b/0x400 [ 5529.508050] ? kobject_put+0x98/0x1d0 [ 5529.508062] usb_disable_device+0xc6/0x1f0 [ 5529.508073] usb_disconnect.cold+0x7e/0x250 [ 5529.508085] hub_event+0xc7b/0x17f0 [ 5529.508099] process_one_work+0x1e3/0x3b0 [ 5529.508109] worker_thread+0x50/0x3b0 [ 5529.508115] ? process_one_work+0x3b0/0x3b0 [ 5529.508122] kthread+0x133/0x160 [ 5529.508127] ? set_kthread_struct+0x40/0x40 [ 5529.508133] ret_from_fork+0x22/0x30 [ 8440.627659] blk_update_request: I/O error, dev sdd, sector 256017240 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 8440.627667] ntfs3: 165 callbacks suppressed [ 8440.627667] ntfs3: sdd1: failed to read volume at offset 0x1e84f6b000 [ 8440.627673] blk_update_request: I/O error, dev sdd, sector 256017240 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 8440.627676] ntfs3: sdd1: failed to read volume at offset 0x1e84f6b000 [ 8440.778355] blk_update_request: I/O error, dev sdd, sector 6293496 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 8440.778384] ntfs3: sdd1: failed to read volume at offset 0xbffff000 [ 8440.778412] blk_update_request: I/O error, dev sdd, sector 6353096 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 8440.778428] ntfs3: sdd1: failed to read volume at offset 0xc1d19000 [ 8440.778441] blk_update_request: I/O error, dev sdd, sector 6353096 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 8440.778452] ntfs3: sdd1: failed to read volume at offset 0xc1d19000 [ 8440.778459] ntfs3: sdd1: ntfs_evict_inode r=0 failed, -22.
On Thursday 29 July 2021 16:49:37 Konstantin Komarov wrote: > diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c > new file mode 100644 > index 000000000..b4369c61a > --- /dev/null > +++ b/fs/ntfs3/file.c > @@ -0,0 +1,1130 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * > + * Copyright (C) 2019-2021 Paragon Software GmbH, All rights reserved. > + * > + * regular file handling primitives for ntfs-based filesystems > + */ > +#include <linux/backing-dev.h> > +#include <linux/buffer_head.h> > +#include <linux/compat.h> > +#include <linux/falloc.h> > +#include <linux/fiemap.h> > +#include <linux/msdos_fs.h> /* FAT_IOCTL_XXX */ > +#include <linux/nls.h> > + > +#include "debug.h" > +#include "ntfs.h" > +#include "ntfs_fs.h" > + > +static int ntfs_ioctl_fitrim(struct ntfs_sb_info *sbi, unsigned long arg) > +{ > + struct fstrim_range __user *user_range; > + struct fstrim_range range; > + struct request_queue *q = bdev_get_queue(sbi->sb->s_bdev); > + int err; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + if (!blk_queue_discard(q)) > + return -EOPNOTSUPP; > + > + user_range = (struct fstrim_range __user *)arg; > + if (copy_from_user(&range, user_range, sizeof(range))) > + return -EFAULT; > + > + range.minlen = max_t(u32, range.minlen, q->limits.discard_granularity); > + > + err = ntfs_trim_fs(sbi, &range); > + if (err < 0) > + return err; > + > + if (copy_to_user(user_range, &range, sizeof(range))) > + return -EFAULT; > + > + return 0; > +} > + > +static long ntfs_ioctl(struct file *filp, u32 cmd, unsigned long arg) > +{ > + struct inode *inode = file_inode(filp); > + struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info; > + u32 __user *user_attr = (u32 __user *)arg; > + > + switch (cmd) { > + case FAT_IOCTL_GET_ATTRIBUTES: > + return put_user(le32_to_cpu(ntfs_i(inode)->std_fa), user_attr); > + > + case FAT_IOCTL_GET_VOLUME_ID: > + return put_user(sbi->volume.ser_num, user_attr); > + > + case FITRIM: > + return ntfs_ioctl_fitrim(sbi, arg); > + } > + return -ENOTTY; /* Inappropriate ioctl for device */ > +} Hello! What with these two FAT_* ioctls in NTFS code? Should NTFS driver really implements FAT ioctls? Because they looks like some legacy API which is even not implemented by current ntfs.ko driver. Specially, should FS driver implements ioctl for get volume id which in this way? Because basically every fs have some kind of uuid / volume id and they can be already retrieved by appropriate userspace tool.
On Sun, Aug 22, 2021 at 02:20:03PM +0200, Pali Rohár wrote: > On Thursday 29 July 2021 16:49:37 Konstantin Komarov wrote: > > diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c > > new file mode 100644 > > index 000000000..b4369c61a > > --- /dev/null > > +++ b/fs/ntfs3/file.c > > @@ -0,0 +1,1130 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * > > + * Copyright (C) 2019-2021 Paragon Software GmbH, All rights reserved. > > + * > > + * regular file handling primitives for ntfs-based filesystems > > + */ > > +#include <linux/backing-dev.h> > > +#include <linux/buffer_head.h> > > +#include <linux/compat.h> > > +#include <linux/falloc.h> > > +#include <linux/fiemap.h> > > +#include <linux/msdos_fs.h> /* FAT_IOCTL_XXX */ > > +#include <linux/nls.h> > > + > > +#include "debug.h" > > +#include "ntfs.h" > > +#include "ntfs_fs.h" > > + > > +static int ntfs_ioctl_fitrim(struct ntfs_sb_info *sbi, unsigned long arg) > > +{ > > + struct fstrim_range __user *user_range; > > + struct fstrim_range range; > > + struct request_queue *q = bdev_get_queue(sbi->sb->s_bdev); > > + int err; > > + > > + if (!capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > + > > + if (!blk_queue_discard(q)) > > + return -EOPNOTSUPP; > > + > > + user_range = (struct fstrim_range __user *)arg; > > + if (copy_from_user(&range, user_range, sizeof(range))) > > + return -EFAULT; > > + > > + range.minlen = max_t(u32, range.minlen, q->limits.discard_granularity); > > + > > + err = ntfs_trim_fs(sbi, &range); > > + if (err < 0) > > + return err; > > + > > + if (copy_to_user(user_range, &range, sizeof(range))) > > + return -EFAULT; > > + > > + return 0; > > +} > > + > > +static long ntfs_ioctl(struct file *filp, u32 cmd, unsigned long arg) > > +{ > > + struct inode *inode = file_inode(filp); > > + struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info; > > + u32 __user *user_attr = (u32 __user *)arg; > > + > > + switch (cmd) { > > + case FAT_IOCTL_GET_ATTRIBUTES: > > + return put_user(le32_to_cpu(ntfs_i(inode)->std_fa), user_attr); > > + > > + case FAT_IOCTL_GET_VOLUME_ID: > > + return put_user(sbi->volume.ser_num, user_attr); > > + > > + case FITRIM: > > + return ntfs_ioctl_fitrim(sbi, arg); > > + } > > + return -ENOTTY; /* Inappropriate ioctl for device */ > > +} > > Hello! What with these two FAT_* ioctls in NTFS code? Should NTFS driver > really implements FAT ioctls? Because they looks like some legacy API > which is even not implemented by current ntfs.ko driver. I was looking same thing when doing new ioctl for shutdown. These should be dropped completly before this gets upstream. Then we have more time to think what ioctl calls should used and which are necessarry. > Specially, should FS driver implements ioctl for get volume id which in > this way? Because basically every fs have some kind of uuid / volume id > and they can be already retrieved by appropriate userspace tool. My first impression when looking this code was that this is just copy paste work from fat driver. FITRIM is exactly the same. Whoever copyed it must have not thinked this very closly. Good thing you bringing this up. I didn't want to just yet because there is quite lot messages and things which are in Komarov todo list. Hopefully radio silence will end soon. I'm afraid next message will be "Please pull" for Linus and then it cannot happend because of radio silence.
On Sunday 22 August 2021 17:31:33 Kari Argillander wrote: > On Sun, Aug 22, 2021 at 02:20:03PM +0200, Pali Rohár wrote: > > On Thursday 29 July 2021 16:49:37 Konstantin Komarov wrote: > > > diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c > > > new file mode 100644 > > > index 000000000..b4369c61a > > > --- /dev/null > > > +++ b/fs/ntfs3/file.c > > > @@ -0,0 +1,1130 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * > > > + * Copyright (C) 2019-2021 Paragon Software GmbH, All rights reserved. > > > + * > > > + * regular file handling primitives for ntfs-based filesystems > > > + */ > > > +#include <linux/backing-dev.h> > > > +#include <linux/buffer_head.h> > > > +#include <linux/compat.h> > > > +#include <linux/falloc.h> > > > +#include <linux/fiemap.h> > > > +#include <linux/msdos_fs.h> /* FAT_IOCTL_XXX */ > > > +#include <linux/nls.h> > > > + > > > +#include "debug.h" > > > +#include "ntfs.h" > > > +#include "ntfs_fs.h" > > > + > > > +static int ntfs_ioctl_fitrim(struct ntfs_sb_info *sbi, unsigned long arg) > > > +{ > > > + struct fstrim_range __user *user_range; > > > + struct fstrim_range range; > > > + struct request_queue *q = bdev_get_queue(sbi->sb->s_bdev); > > > + int err; > > > + > > > + if (!capable(CAP_SYS_ADMIN)) > > > + return -EPERM; > > > + > > > + if (!blk_queue_discard(q)) > > > + return -EOPNOTSUPP; > > > + > > > + user_range = (struct fstrim_range __user *)arg; > > > + if (copy_from_user(&range, user_range, sizeof(range))) > > > + return -EFAULT; > > > + > > > + range.minlen = max_t(u32, range.minlen, q->limits.discard_granularity); > > > + > > > + err = ntfs_trim_fs(sbi, &range); > > > + if (err < 0) > > > + return err; > > > + > > > + if (copy_to_user(user_range, &range, sizeof(range))) > > > + return -EFAULT; > > > + > > > + return 0; > > > +} > > > + > > > +static long ntfs_ioctl(struct file *filp, u32 cmd, unsigned long arg) > > > +{ > > > + struct inode *inode = file_inode(filp); > > > + struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info; > > > + u32 __user *user_attr = (u32 __user *)arg; > > > + > > > + switch (cmd) { > > > + case FAT_IOCTL_GET_ATTRIBUTES: > > > + return put_user(le32_to_cpu(ntfs_i(inode)->std_fa), user_attr); > > > + > > > + case FAT_IOCTL_GET_VOLUME_ID: > > > + return put_user(sbi->volume.ser_num, user_attr); > > > + > > > + case FITRIM: > > > + return ntfs_ioctl_fitrim(sbi, arg); > > > + } > > > + return -ENOTTY; /* Inappropriate ioctl for device */ > > > +} > > > > Hello! What with these two FAT_* ioctls in NTFS code? Should NTFS driver > > really implements FAT ioctls? Because they looks like some legacy API > > which is even not implemented by current ntfs.ko driver. > > I was looking same thing when doing new ioctl for shutdown. These > should be dropped completly before this gets upstream. Then we have > more time to think what ioctl calls should used and which are > necessarry. Ok. I agree, these FAT* ioctls should not be included into upstream in this way. Later we can decide how to handle them... > > Specially, should FS driver implements ioctl for get volume id which in > > this way? Because basically every fs have some kind of uuid / volume id > > and they can be already retrieved by appropriate userspace tool. > > My first impression when looking this code was that this is just copy > paste work from fat driver. FITRIM is exactly the same. Whoever > copyed it must have not thinked this very closly. Good thing you > bringing this up. > > I didn't want to just yet because there is quite lot messages and > things which are in Komarov todo list. Hopefully radio silence will > end soon. I'm afraid next message will be "Please pull" for Linus > and then it cannot happend because of radio silence. >