diff mbox series

[v4,12/13] ext4: Stop providing .writepage hook

Message ID 20221207112722.22220-12-jack@suse.cz
State Accepted
Headers show
Series ext4: Stop using ext4_writepage() for writeout of ordered data | expand

Commit Message

Jan Kara Dec. 7, 2022, 11:27 a.m. UTC 
Now we don't need .writepage hook for anything anymore. Reclaim is fine
with relying on .writepages to clean pages and we often couldn't do much
from the .writepage callback anyway. We only need to provide
.migrate_folio callback for the ext4_journalled_aops - let's use
buffer_migrate_page_norefs() there so that buffers cannot be modified
under jdb2's hands as that can cause data corruption. For example when
commit code does writeout of transaction buffers in
jbd2_journal_write_metadata_buffer(), we don't hold page lock or have
page writeback bit set or have the buffer locked. So page migration code
would go and happily migrate the page elsewhere while the copy is
running thus corrupting data.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

youling 257 May 8, 2023, 5:51 p.m. UTC | #1 
I using linux mainline kernel on android. https://github.com/youling257/android-mainline/commits/6.4  https://github.com/youling257/android-mainline/commits/6.3
"ext4: Stop providing .writepage hook" cause some android app unable to read storage/emulated/0 files, i need to say android esdfs file system storage/emulated is ext4 data/media bind mount.
I want to ask, why android storage/emulated need .writepage hook?
Jan Kara May 9, 2023, 12:25 a.m. UTC | #2 
Hello,

On Tue 09-05-23 01:51:08, youling257 wrote:
> I using linux mainline kernel on android.
> https://github.com/youling257/android-mainline/commits/6.4
> https://github.com/youling257/android-mainline/commits/6.3 "ext4: Stop
> providing .writepage hook" cause some android app unable to read
> storage/emulated/0 files, i need to say android esdfs file system
> storage/emulated is ext4 data/media bind mount.  I want to ask, why
> android storage/emulated need .writepage hook?

Honestly, I don't know. I guess you need to look into implementation of
esdfs in the Android kernel and why it needs filesystem's .writepage
hook...

								Honza
Eric Biggers May 9, 2023, 5:02 a.m. UTC | #3 
On Tue, May 09, 2023 at 01:51:08AM +0800, youling257 wrote:
> I using linux mainline kernel on android. https://github.com/youling257/android-mainline/commits/6.4  https://github.com/youling257/android-mainline/commits/6.3
> "ext4: Stop providing .writepage hook" cause some android app unable to read storage/emulated/0 files, i need to say android esdfs file system storage/emulated is ext4 data/media bind mount.
> I want to ask, why android storage/emulated need .writepage hook?

"esdfs" doesn't exist upstream, so linux-ext4 can't provide support for it.

Also, it doesn't exist in the Android Common Kernels either, so the Android team
cannot help you either.

- Eric
Theodore Ts'o May 9, 2023, 6:36 p.m. UTC | #4 
On Mon, May 08, 2023 at 10:02:27PM -0700, Eric Biggers wrote:
> On Tue, May 09, 2023 at 01:51:08AM +0800, youling257 wrote:
> > I using linux mainline kernel on android. https://github.com/youling257/android-mainline/commits/6.4  https://github.com/youling257/android-mainline/commits/6.3
> > "ext4: Stop providing .writepage hook" cause some android app unable to read storage/emulated/0 files, i need to say android esdfs file system storage/emulated is ext4 data/media bind mount.
> > I want to ask, why android storage/emulated need .writepage hook?
> 
> "esdfs" doesn't exist upstream, so linux-ext4 can't provide support for it.
> 
> Also, it doesn't exist in the Android Common Kernels either, so the Android team
> cannot help you either.

The problem with esdfs is that it's based on the old stackable file
system paradigm which is filled with races and is inherently
unreliable (just for fun, try running fsstress on the upper and lower
file systems of a stackable file system simultaneously, and watch the
kernel crash and burn).  For that reason, some number of us have been
working for a while to eliminate the need for stacking file systems,
such as sdcardfs. esdfs, etc. from the Android kernel.

The other thing I would add is that upstream has been working[1] on
getting rid of writepage function.  So out-of-tree file systems are
going to need to adapt --- or die.

[1] https://lore.kernel.org/all/20221202102644.770505-1-hch@lst.de/

It looks like esdfs is coming from the Chromium kernel?  The latest
Chromium kernel I can find is 5.15 based, and it has esdfs in it.  I'm
sad to see that esdfs hasn't been removed from the Chromium kernel
yet, and replaced with something more stable and reliable, but maybe
we can find someone who is more familiar with the Chromium kernel to
comment.

Cheers,

						- Ted
youling 257 May 10, 2023, 5:17 a.m. UTC | #5 
It isn't android storage emulated esdfs or sdcardfs problem, i test
mount bind /data/media on /storage/emulated, can reproduce my problem.
Let me say clear my problem, "ext4: Stop providing .writepage hook"
cause android gallery app unable read pictures thumbnails.

I test linux kernel 6.3 Revert "ext4: Stop providing .writepage hook",
android gallery can read pictures thumbnails,

this is mount bind /data/media on /storage/emulated
/dev/nvme0n1p1 on /storage/emulated type ext4 (rw,seclabel,noatime)
/dev/nvme0n1p1 on /mnt/runtime/default/emulated type ext4 (rw,seclabel,noatime)
/dev/nvme0n1p1 on /mnt/runtime/read/emulated type ext4 (rw,seclabel,noatime)
/dev/nvme0n1p1 on /mnt/runtime/write/emulated type ext4 (rw,seclabel,noatime)
/dev/nvme0n1p1 on /mnt/runtime/full/emulated type ext4 (rw,seclabel,noatime)

this is esdfs,
/data/media on /mnt/runtime/default/emulated type esdfs
(rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:1015:771:771,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
/data/media on /storage/emulated type esdfs
(rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:1015:771:771,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
/data/media on /mnt/runtime/read/emulated type esdfs
(rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:9997:750:750,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
/data/media on /mnt/runtime/write/emulated type esdfs
(rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:9997:770:770,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
/data/media on /mnt/runtime/full/emulated type esdfs
(rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:9997:770:770,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)

if i do mount bind data/media on storage/emulated, take a screenshot,
will create /data/media/0/Pictures/Screenshots/Screenshot_20230510-130020.png
file,
chmod -R 0777 /data/media/0/Pictures/Screenshots,
on linux 6.3 Revert "ext4: Stop providing .writepage hook", android
gallery app can read
/storage/emulated/0/Pictures/Screenshots/Screenshot_20230510-130020.png
thumbnail,
linux 6.4 removed .writepage hook, android gallery unable read thumbnail.


2023-05-10 2:36 GMT+08:00, Theodore Ts'o <tytso@mit.edu>:
> On Mon, May 08, 2023 at 10:02:27PM -0700, Eric Biggers wrote:
>> On Tue, May 09, 2023 at 01:51:08AM +0800, youling257 wrote:
>> > I using linux mainline kernel on android.
>> > https://github.com/youling257/android-mainline/commits/6.4
>> > https://github.com/youling257/android-mainline/commits/6.3
>> > "ext4: Stop providing .writepage hook" cause some android app unable to
>> > read storage/emulated/0 files, i need to say android esdfs file system
>> > storage/emulated is ext4 data/media bind mount.
>> > I want to ask, why android storage/emulated need .writepage hook?
>>
>> "esdfs" doesn't exist upstream, so linux-ext4 can't provide support for
>> it.
>>
>> Also, it doesn't exist in the Android Common Kernels either, so the
>> Android team
>> cannot help you either.
>
> The problem with esdfs is that it's based on the old stackable file
> system paradigm which is filled with races and is inherently
> unreliable (just for fun, try running fsstress on the upper and lower
> file systems of a stackable file system simultaneously, and watch the
> kernel crash and burn).  For that reason, some number of us have been
> working for a while to eliminate the need for stacking file systems,
> such as sdcardfs. esdfs, etc. from the Android kernel.
>
> The other thing I would add is that upstream has been working[1] on
> getting rid of writepage function.  So out-of-tree file systems are
> going to need to adapt --- or die.
>
> [1] https://lore.kernel.org/all/20221202102644.770505-1-hch@lst.de/
>
> It looks like esdfs is coming from the Chromium kernel?  The latest
> Chromium kernel I can find is 5.15 based, and it has esdfs in it.  I'm
> sad to see that esdfs hasn't been removed from the Chromium kernel
> yet, and replaced with something more stable and reliable, but maybe
> we can find someone who is more familiar with the Chromium kernel to
> comment.
>
> Cheers,
>
> 						- Ted
>
youling 257 May 10, 2023, 5:47 a.m. UTC | #6 
I do more test, it is android esdfs or sdcardfs
/storage/emulated/0/Android/data problem,
"ext4: Stop providing .writepage hook" cause
/storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.0
/storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.1
/storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.idx
unable read,

on linux 6.4, i use mount bind data/media on storage/emulated, chmod
-R 0777 /data/media/0, rm
/storage/emulated/0/Android/data/com.android.gallery3d/cache/*, open
gallery app can read pictures thumbnail,
/storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.idx
/storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.0
/storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.1
available read.


2023-05-10 13:17 GMT+08:00, youling 257 <youling257@gmail.com>:
> It isn't android storage emulated esdfs or sdcardfs problem, i test
> mount bind /data/media on /storage/emulated, can reproduce my problem.
> Let me say clear my problem, "ext4: Stop providing .writepage hook"
> cause android gallery app unable read pictures thumbnails.
>
> I test linux kernel 6.3 Revert "ext4: Stop providing .writepage hook",
> android gallery can read pictures thumbnails,
>
> this is mount bind /data/media on /storage/emulated
> /dev/nvme0n1p1 on /storage/emulated type ext4 (rw,seclabel,noatime)
> /dev/nvme0n1p1 on /mnt/runtime/default/emulated type ext4
> (rw,seclabel,noatime)
> /dev/nvme0n1p1 on /mnt/runtime/read/emulated type ext4
> (rw,seclabel,noatime)
> /dev/nvme0n1p1 on /mnt/runtime/write/emulated type ext4
> (rw,seclabel,noatime)
> /dev/nvme0n1p1 on /mnt/runtime/full/emulated type ext4
> (rw,seclabel,noatime)
>
> this is esdfs,
> /data/media on /mnt/runtime/default/emulated type esdfs
> (rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:1015:771:771,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
> /data/media on /storage/emulated type esdfs
> (rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:1015:771:771,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
> /data/media on /mnt/runtime/read/emulated type esdfs
> (rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:9997:750:750,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
> /data/media on /mnt/runtime/write/emulated type esdfs
> (rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:9997:770:770,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
> /data/media on /mnt/runtime/full/emulated type esdfs
> (rw,nosuid,nodev,noexec,noatime,lower=1023:1023:664:775,upper=0:9997:770:770,derive=multi,noconfine,derive_gid,default_normal,unshared_obb)
>
> if i do mount bind data/media on storage/emulated, take a screenshot,
> will create
> /data/media/0/Pictures/Screenshots/Screenshot_20230510-130020.png
> file,
> chmod -R 0777 /data/media/0/Pictures/Screenshots,
> on linux 6.3 Revert "ext4: Stop providing .writepage hook", android
> gallery app can read
> /storage/emulated/0/Pictures/Screenshots/Screenshot_20230510-130020.png
> thumbnail,
> linux 6.4 removed .writepage hook, android gallery unable read thumbnail.
>
>
> 2023-05-10 2:36 GMT+08:00, Theodore Ts'o <tytso@mit.edu>:
>> On Mon, May 08, 2023 at 10:02:27PM -0700, Eric Biggers wrote:
>>> On Tue, May 09, 2023 at 01:51:08AM +0800, youling257 wrote:
>>> > I using linux mainline kernel on android.
>>> > https://github.com/youling257/android-mainline/commits/6.4
>>> > https://github.com/youling257/android-mainline/commits/6.3
>>> > "ext4: Stop providing .writepage hook" cause some android app unable
>>> > to
>>> > read storage/emulated/0 files, i need to say android esdfs file system
>>> > storage/emulated is ext4 data/media bind mount.
>>> > I want to ask, why android storage/emulated need .writepage hook?
>>>
>>> "esdfs" doesn't exist upstream, so linux-ext4 can't provide support for
>>> it.
>>>
>>> Also, it doesn't exist in the Android Common Kernels either, so the
>>> Android team
>>> cannot help you either.
>>
>> The problem with esdfs is that it's based on the old stackable file
>> system paradigm which is filled with races and is inherently
>> unreliable (just for fun, try running fsstress on the upper and lower
>> file systems of a stackable file system simultaneously, and watch the
>> kernel crash and burn).  For that reason, some number of us have been
>> working for a while to eliminate the need for stacking file systems,
>> such as sdcardfs. esdfs, etc. from the Android kernel.
>>
>> The other thing I would add is that upstream has been working[1] on
>> getting rid of writepage function.  So out-of-tree file systems are
>> going to need to adapt --- or die.
>>
>> [1] https://lore.kernel.org/all/20221202102644.770505-1-hch@lst.de/
>>
>> It looks like esdfs is coming from the Chromium kernel?  The latest
>> Chromium kernel I can find is 5.15 based, and it has esdfs in it.  I'm
>> sad to see that esdfs hasn't been removed from the Chromium kernel
>> yet, and replaced with something more stable and reliable, but maybe
>> we can find someone who is more familiar with the Chromium kernel to
>> comment.
>>
>> Cheers,
>>
>> 						- Ted
>>
>
Eric Biggers May 10, 2023, 6:50 a.m. UTC | #7 
On Wed, May 10, 2023 at 01:47:58PM +0800, youling 257 wrote:
> I do more test, it is android esdfs or sdcardfs
> /storage/emulated/0/Android/data problem,
> "ext4: Stop providing .writepage hook" cause
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.0
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.1
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.idx
> unable read,
> 
> on linux 6.4, i use mount bind data/media on storage/emulated, chmod
> -R 0777 /data/media/0, rm
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/*, open
> gallery app can read pictures thumbnail,
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.idx
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.0
> /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.1
> available read.

Maybe try reverting your commit that added esdfs to your kernel?  It should not
be needed at all.

- Eric
Theodore Ts'o May 10, 2023, 10 p.m. UTC | #8 
On Tue, May 09, 2023 at 11:50:36PM -0700, Eric Biggers wrote:
> On Wed, May 10, 2023 at 01:47:58PM +0800, youling 257 wrote:
> > I do more test, it is android esdfs or sdcardfs
> > /storage/emulated/0/Android/data problem,
> > "ext4: Stop providing .writepage hook" cause
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.0
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.1
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.idx
> > unable read,
> > 
> > on linux 6.4, i use mount bind data/media on storage/emulated, chmod
> > -R 0777 /data/media/0, rm
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/*, open
> > gallery app can read pictures thumbnail,
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.idx
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.0
> > /storage/emulated/0/Android/data/com.android.gallery3d/cache/imgcache.1
> > available read.
> 
> Maybe try reverting your commit that added esdfs to your kernel?  It should not
> be needed at all.

Youling, what version of Android are you trying to run with the latest
bleeding edge kernel?  Starting with Android 11, sdcardfs was
deprecated[1].

    SDCardFS is deprecated on devices that launch with Android 11 or
    higher and run kernel version 5.4 or higher. On such devices, VTS
    testing doesn't allow mounted file systems listed as
    SDCardFS. Devices that launch with Android 11 or higher but run
    kernel version 4.19 or lower can continue to use SDCardFS, but
    Google doesn't provide additional support.

[1] https://source.android.com/docs/core/storage/sdcardfs-deprecate

With newer versions of Android, use of something like sdcardfs or
esdfs is not necessary.  If you are using an older version of Android,
and you insist on use a bleeding edge kernel where the writepage is
getting deprecated, then someone will need to update esdfs or deal
with the changing internal interfaces of the upstream kernel.  This is
not the ext4 upstream developer's problem.

Personally, I would recommend that you *not* try to fix esdfs; that's
because stacking file systems like sdcardfs and esdfs are inherently
unreliable.   See the section in [1] entitled, "Why deprecate sdcardfs?".

Instead, what I would recommend is upgrading to a newer version of
Android, and then dropping esdfs from your kernel repository.

	     	  	   	      	   	  - Ted
diff mbox series

Patch

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f3b3792c1c96..acf9d23c1cfb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3718,7 +3718,6 @@  static int ext4_iomap_swap_activate(struct swap_info_struct *sis,
 static const struct address_space_operations ext4_aops = {
 	.read_folio		= ext4_read_folio,
 	.readahead		= ext4_readahead,
-	.writepage		= ext4_writepage,
 	.writepages		= ext4_writepages,
 	.write_begin		= ext4_write_begin,
 	.write_end		= ext4_write_end,
@@ -3736,7 +3735,6 @@  static const struct address_space_operations ext4_aops = {
 static const struct address_space_operations ext4_journalled_aops = {
 	.read_folio		= ext4_read_folio,
 	.readahead		= ext4_readahead,
-	.writepage		= ext4_writepage,
 	.writepages		= ext4_writepages,
 	.write_begin		= ext4_write_begin,
 	.write_end		= ext4_journalled_write_end,
@@ -3745,6 +3743,7 @@  static const struct address_space_operations ext4_journalled_aops = {
 	.invalidate_folio	= ext4_journalled_invalidate_folio,
 	.release_folio		= ext4_release_folio,
 	.direct_IO		= noop_direct_IO,
+	.migrate_folio		= buffer_migrate_folio_norefs,
 	.is_partially_uptodate  = block_is_partially_uptodate,
 	.error_remove_page	= generic_error_remove_page,
 	.swap_activate		= ext4_iomap_swap_activate,
@@ -3753,7 +3752,6 @@  static const struct address_space_operations ext4_journalled_aops = {
 static const struct address_space_operations ext4_da_aops = {
 	.read_folio		= ext4_read_folio,
 	.readahead		= ext4_readahead,
-	.writepage		= ext4_writepage,
 	.writepages		= ext4_writepages,
 	.write_begin		= ext4_da_write_begin,
 	.write_end		= ext4_da_write_end,