Re: [PATCH v2 3/3] ntfs: do not convert a named $DATA stream when making room in a record

[Wahid Khan <wahidzk0091@gmail.com>]

Hi Namjae,

I sent v3 of this patch as a reply to this mail. It opens a named $DATA
stream with ntfs_attr_iget() like the other attribute types in the loop,
and keeps ntfs_iget() for the unnamed one. With it the image that
triggered the WARN mounts and walks cleanly on a KASAN and lockdep
kernel. Without it the WARN fires at ntfs_attr_make_non_resident().

About the lock ordering: I checked the code. The comment above the
__filemap_get_folio() call in ntfs_attr_make_non_resident() says the
folio lock nests outside all ntfs locks, but its callers hold mrec_lock
when they get there (ntfs_expand_for_write() takes it in file.c and goes
through ntfs_attr_expand() into ntfs_resident_attr_resize()). The read
side takes the folio first and mrec_lock inside
ntfs_read_iomap_begin_resident(). So the two orders do exist. This is
true for the direct conversion at the top of ntfs_resident_attr_resize()
as well, not only for the loop.

I could not turn it into a hang. A reader and a writer racing on resident
files, also with a delay forced between taking mrec_lock and the folio,
completed every time. So I do not know how reachable it is, and I did
not want to change the locking in this patch on a guess.

If you want the order fixed, I think it needs the folio to be taken
before mrec_lock in the callers (or a non-blocking folio get on the
make-room path, which only tries to free space). Tell me which you
prefer and I will send it as a separate patch.

Thanks,
Wahid