Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 8 Aug 2015 11:41:21 +0300
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Gleb Smirnoff <glebius@FreeBSD.org>
Cc:        arch@FreeBSD.org, alc@freebsd.org
Subject:   Re: more strict KPI for vm_pager_get_pages()
Message-ID:  <20150808084121.GX2072@kib.kiev.ua>
In-Reply-To: <20150807133844.GS889@FreeBSD.org>
References:  <20150430142408.GS546@nginx.com> <20150807133844.GS889@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Aug 07, 2015 at 04:38:44PM +0300, Gleb Smirnoff wrote:
>   Hi!
> 
>   This is followup on older email:
> 
> https://lists.freebsd.org/pipermail/freebsd-arch/2015-April/017154.html
> 
> The preparatory commits were already checked in, and I'm going
> to push next week the rest, since the whole story is already
> several months due.
> 
> Planned changes:
> 
> o vm_pager_get_pages() accepts array of pages, and treats all pages
>   equally. Notion of reqpage goes away.
> o The array span validity must be checked before with vm_pager_has_page().
> o All pages must be xbusied on enter.
> o All pages will be left xbusied on exit. This closes possible races,
>   allows to pass in wired pages (for any pager). And it leaves
>   the caller to decide what to do with pages: vm_page_active,
>   vm_page_deactivate, vm_page_flash or just vm_page_free them.
> 
> The patch has been tested by me and pho@ with his stress2 test.
> 
> I know, that there are two comments from kib@ on the patch.
These were not comments, but objections.

> 
> 1) There could be a user of KPI who would be fine with partial success.
> 
> My answer: right now there is none, and if one emerges, the code can
> be easily adopted to return VM_PAGER_ERROR, but still mark validated
> pages as valid. The user of KPI then can scan the array and take valid
> pages. So, the patch doesn't put any obstacles on appearance of such
> user.
The vm_fault.c is already fine with the partial success, it only cares
that the requested page was validated and no error from pager is
returned.

> 
> 2) Filesystems can do short reads by design, and thus fail to validate
>    the entire array.
> 
> My answer: yes, that's true. By design NFS, SMBFS and FUSE should be
> able to return short reads. However, the VOP_GETPAGES methods for all
> three FSes right now do not have any code that would support that. So,
> it looks like there is an open issue with these filesystems, not related
> to my patch. When this issue is addressed in any of aforementioned FSes,
> the VOP_GETPAGES should be fixed to do several I/Os in case of short
> reads.

And this is a bug in the networking filesystems (most likely).  Rick was
asked about NFS, but he did not responded.  You are proposing to make the
bug a part of the interface.

I object against this change. It is wrong philosophically, and it
encodes the incomplete or accidental behaviour of several filesystems at
the interface level.

In fact, you are taking some rather secondary feature of the current
interface, that the non-requested pages are made busy for the duration
of the paging request, to the level of the fundamental property (would
the non-mreq pages be not busied, proposed change immediately causes
applications segfault on parallel file truncation, or makes user data
corrupted, for example).

All this rototilling is because you do not want to code the proper FSA
in your reworked sendfile patch.

I object against the change and against the reasoning behind it.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150808084121.GX2072>