Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled

[Kari Argillander <kari.argillander@gmail.com>]

On Sun, Oct 24, 2021 at 08:35:57AM +0000, Ganapathi Kamath wrote:
> Hellom 
> 
> While compiling, first time around, I got the below.

Yeah. Patch was meant for ntfs3/master...

> fs/ntfs3/file.c: In function 'ntfs_truncate': 
> fs/ntfs3/file.c:498:60: error: invalid type argument of '->' (have 'struct ntfs_mount_options')
>   498 |                             &new_valid, ni->mi.sbi->options->prealloc, NULL);
>       |                                                            ^~
> make[2]: *** [scripts/Makefile.build:277: fs/ntfs3/file.o] Error 1
> make[1]: *** [scripts/Makefile.build:540: fs/ntfs3] Error 2
> 
> So, in the file
>     fs/ntfs3/file.c
> I changed 
>     ni->mi.sbi->options->prealloc
> to
>     ni->mi.sbi->options.prealloc

but with your change it also applies to rc3. Your change was correct and
it was totally ok to test top of rc3.

> I don't really follow/understand the code, to understand what exactly
> the logic is, except that you are trying to set boolean
> 'keep_prealloc' call-argument for attr_set_size() by using the
> ntfs_mount_option bit-field 'prealloc' which is to "Preallocate space
> when file is growing", prototyped in the file fs/ntfs3/ntfs_fs.h
> 
> * Built new 5.15.0-0.rc3.20211001git4de593fb965f.30.fc35.x86_64
> kernel. (4 hrs on my machine)

I do not know if you used -j flag when making. Just if you did not know
about it use:

  make -j8

This will example use 8 threads for compiling. You choose number based
on how many threads you have in your processor.

> * I was able to include patch into rpmbuild of kernel src patch, with
> aforementioned correction * first reconfirmed/verified bug on old
> kernel
> * installed newly built kernel
> * attempt reproduction no success meaning bug not present on new
> kernel, patch/fix makes file size on overwrite to be as expected.
> 
> note, I am not an expert, and as a user, I don't know 100% what
> correct behavior should be, only what seems reasonable expected
> behavior, But  you are experts, so please excuse me for reiterating
> what you know. NTFS is a filesystem that was designed by microsoft for
> windows, and the way its fs-driver must update is so that on-disk
> structures is suitable for windows in-kernel structures. A kernel
> driver for linux, only adapts on disk-ntfs structures to something
> suitable for linux in-kernel structures, but must update the on disk
> structures the way Windows expects/designed it to.

Ntfs file structure allows actually many things which even Windows does
not understand. This is new driver and it will be evolving that user can
decide what he wants. We will also have to define good defaults so that
user get good experience.

> So if you defended old behavior, I wouldn't know.  So its your call,
> to decide if it is a bug, and whether your patch fixes.  On my side,
> my machine is one I work on. patch seems to fix claimed bug. So I hope
> there is no side effect, nothing corrupts or becomes unstable. 

This was bug. Thanks for reporting and testing it.  We really appriciate
it. This is new driver and there will be bugs so early users who report
bugs and are even willing to test patches are gold mine for us.

We will also add tags to patch:
Reported-by: Ganapathi Kamath <hgkamath@hotmail.com>
Tested-by: Ganapathi Kamath <hgkamath@hotmail.com>

if it ok to you. This way if you report new bugs to kernel people will
know you are good reporter as you have also tested what you have
reported. If you wanna know more about these tags see [1].

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

  Argillander

> That was fast fix. congrats. 
> 
> Log:
> [root@sirius gana]#
> [root@sirius gana]# mount -t ntfs3 /dev/sda17 /mnt/a17/
> [root@sirius gana]#
> [root@sirius gana]# rm -f /mnt/a17/test1.bin /mnt/a17/test2.bin
> [root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test2.bin bs=1M count=3000
> 3000+0 records in
> 3000+0 records out
> 3145728000 bytes (3.1 GB, 2.9 GiB) copied, 5.40015 s, 583 MB/s
> [root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test1.bin bs=1M count=6000
> 6000+0 records in
> 6000+0 records out
> 6291456000 bytes (6.3 GB, 5.9 GiB) copied, 16.1809 s, 389 MB/s
> [root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
> 6144000 -rw-r--r--. 1 root root 6291456000 Oct 24 13:42 /mnt/a17/test1.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
> [root@sirius gana]# cp /mnt/a17/test2.bin /mnt/a17/test1.bin
> cp: overwrite '/mnt/a17/test1.bin'? y
> [root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:42 /mnt/a17/test1.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
> [root@sirius gana]#  stat /mnt/a17/test1.bin
>   File: /mnt/a17/test1.bin
>   Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
> Device: 10301h/66305d   Inode: 44          Links: 1
> Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2021-10-24 13:41:59.265503300 +0530
> Modify: 2021-10-24 13:42:44.738904000 +0530
> Change: 2021-10-24 13:42:44.738904000 +0530
>  Birth: 2021-10-24 13:41:59.265503300 +0530
> [root@sirius gana]#  stat /mnt/a17/test2.bin
>   File: /mnt/a17/test2.bin
>   Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
> Device: 10301h/66305d   Inode: 43          Links: 1
> Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2021-10-24 13:42:40.610776900 +0530
> Modify: 2021-10-24 13:41:52.684315600 +0530
> Change: 2021-10-24 13:41:52.684315600 +0530
>  Birth: 2021-10-24 13:41:47.284266100 +0530