Linux: fix zfs_uio_dio_check_for_zero_page

[snajpa]

Motivation and Context

#16689 #16642

Description

Linux: fix zfs_uio_dio_check_for_zero_page

The intent here is to replace the zero page pointer in the array of
pointers to pages in the struct.

How Has This Been Tested?

locally with reproducer in #16689

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

The fix makes sense to me.

I am just curios about all this special case: why do we need custom zero page? Can system-wide zero page become non-zero somehow? We are talking about write here, not read.

[mtippmann]

not directly related but I'm just wondering is running tests additinally with init_on_alloc=0 init_on_free=0 would have catched that earlier? not sure how common usage of this is but wisdom was it's a notable difference in performance.

[snajpa]

The fix makes sense to me.

I am just curios about all this special case: why do we need custom zero page? Can system-wide zero page become non-zero somehow? We are talking about write here, not read.

AFAIK this is because the zero_page symbol is now GPL-only

[amotin]

AFAIK this is because the zero_page symbol is now GPL-only

That is not what I asked. Why do we even need to know that some page is zero page?

[snajpa]

Well there's this:

			/*
			 * If the user page points the kernels ZERO_PAGE() a
			 * new zero filled page will just be allocated so the
			 * contents of the page can not be changed by the user
			 * while a Direct I/O write is taking place.
			 */

which I just took at face value, but I honestly have no idea how it might happen, the contents being changed by the user, if we didn't do this at all... because ifdef HAVE_ZERO_PAGE_GPL_ONLY, we just don't do it?

and also there's the typo of ZFS_MARKEED_PAGE...

@bwatkinson can you remember any details about this? What if we nuke all these lines of code that are about the zero page? What bad would happen, how can a user change the uio dio buffer content and am I reading it right that the protection isn't really there ifdef HAVE_ZERO_PAGE_GPL_ONLY? I don't hit the path b/c I'm running with gpl_is_licence_compatible(CDDL) == true...

[behlendorf]

What if we nuke all these lines of code that are about the zero page?

We want to keep the zero page check here to handle this case. I suspect we could end up with a zero page in the draid case where we're padding out a write which is smaller than the minimum allocation size.

and also there's the typo of ZFS_MARKEED_PAGE.

Would you mind opening a PR to fix this typo too.