| Message ID | 20200418225123.31850-1-guoqing.jiang@cloud.ionos.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | export __clear_page_buffers to cleanup code | expand |
On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: > When reading md code, I find md-bitmap.c copies __clear_page_buffers from > buffer.c, and after more search, seems there are some places in fs could > use this function directly. So this patchset tries to export the function > and use it to cleanup code. OK, I see why you did this, but there are a couple of problems with it. One is just a sequencing problem; between exporting __clear_page_buffers() and removing it from the md code, the md code won't build. More seriously, most of this code has nothing to do with buffers. It uses page->private for its own purposes. What I would do instead is add: clear_page_private(struct page *page) { ClearPagePrivate(page); set_page_private(page, 0); put_page(page); } to include/linux/mm.h, then convert all callers of __clear_page_buffers() to call that instead.
On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote: > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: > > When reading md code, I find md-bitmap.c copies __clear_page_buffers from > > buffer.c, and after more search, seems there are some places in fs could > > use this function directly. So this patchset tries to export the function > > and use it to cleanup code. > > OK, I see why you did this, but there are a couple of problems with it. > > One is just a sequencing problem; between exporting __clear_page_buffers() > and removing it from the md code, the md code won't build. > > More seriously, most of this code has nothing to do with buffers. It > uses page->private for its own purposes. > > What I would do instead is add: > > clear_page_private(struct page *page) > { > ClearPagePrivate(page); > set_page_private(page, 0); > put_page(page); > } > > to include/linux/mm.h, then convert all callers of __clear_page_buffers() > to call that instead. Agreed with the new naming (__clear_page_buffers is confusing), that is not only for initial use buffer head, but a generic convention for all unlocked PagePrivate pages (such migration & reclaim paths indicate that). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mm.h?h=v5.7-rc1#n990 Thanks, Gao Xiang >
On 19.04.20 05:14, Matthew Wilcox wrote: > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: >> When reading md code, I find md-bitmap.c copies __clear_page_buffers from >> buffer.c, and after more search, seems there are some places in fs could >> use this function directly. So this patchset tries to export the function >> and use it to cleanup code. > OK, I see why you did this, but there are a couple of problems with it. > > One is just a sequencing problem; between exporting __clear_page_buffers() > and removing it from the md code, the md code won't build. Thank for reminder, I missed that. > More seriously, most of this code has nothing to do with buffers. It > uses page->private for its own purposes. > > What I would do instead is add: > > clear_page_private(struct page *page) > { > ClearPagePrivate(page); > set_page_private(page, 0); > put_page(page); > } > > to include/linux/mm.h, then convert all callers of __clear_page_buffers() > to call that instead. > Thanks for your suggestion! Thanks, Guoqing
On 19.04.20 07:14, Gao Xiang wrote: > On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote: >> On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: >>> When reading md code, I find md-bitmap.c copies __clear_page_buffers from >>> buffer.c, and after more search, seems there are some places in fs could >>> use this function directly. So this patchset tries to export the function >>> and use it to cleanup code. >> OK, I see why you did this, but there are a couple of problems with it. >> >> One is just a sequencing problem; between exporting __clear_page_buffers() >> and removing it from the md code, the md code won't build. >> >> More seriously, most of this code has nothing to do with buffers. It >> uses page->private for its own purposes. >> >> What I would do instead is add: >> >> clear_page_private(struct page *page) >> { >> ClearPagePrivate(page); >> set_page_private(page, 0); >> put_page(page); >> } >> >> to include/linux/mm.h, then convert all callers of __clear_page_buffers() >> to call that instead. > Agreed with the new naming (__clear_page_buffers is confusing), that is not > only for initial use buffer head, but a generic convention for all unlocked > PagePrivate pages (such migration & reclaim paths indicate that). > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mm.h?h=v5.7-rc1#n990 Thanks for the link, and will rename the function to clear_page_private. Thanks, Guoqing
Hi Mattew, On 19.04.20 05:14, Matthew Wilcox wrote: > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: >> When reading md code, I find md-bitmap.c copies __clear_page_buffers from >> buffer.c, and after more search, seems there are some places in fs could >> use this function directly. So this patchset tries to export the function >> and use it to cleanup code. > OK, I see why you did this, but there are a couple of problems with it. > > One is just a sequencing problem; between exporting __clear_page_buffers() > and removing it from the md code, the md code won't build. Seems the build option BLK_DEV_MD is depended on BLOCK, and buffer.c is relied on the same option. ifeq ($(CONFIG_BLOCK),y)/x obj-y += buffer.o block_dev.o direct-io.o mpage.o else obj-y += no-block.o endif So I am not sure it is necessary to move the function to include/linux/mm.h if there is no sequencing problem, thanks for your any further suggestion. Thanks, Guoqing
On Sun, Apr 19, 2020 at 10:31:26PM +0200, Guoqing Jiang wrote: > On 19.04.20 05:14, Matthew Wilcox wrote: > > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: > > > When reading md code, I find md-bitmap.c copies __clear_page_buffers from > > > buffer.c, and after more search, seems there are some places in fs could > > > use this function directly. So this patchset tries to export the function > > > and use it to cleanup code. > > OK, I see why you did this, but there are a couple of problems with it. > > > > One is just a sequencing problem; between exporting __clear_page_buffers() > > and removing it from the md code, the md code won't build. > > Seems the build option BLK_DEV_MD is depended on BLOCK, and buffer.c > is relied on the same option. > > ifeq ($(CONFIG_BLOCK),y)/x > obj-y += buffer.o block_dev.o direct-io.o mpage.o > else > obj-y += no-block.o > endif > > So I am not sure it is necessary to move the function to include/linux/mm.h > if there is no sequencing problem, thanks for your any further suggestion. The sequencing problem is that there will be _two_ definitions of __clear_page_buffers(). The reason it should go in mm.h is that it's a very simple function and it will be better to inline it than call an external function. The two things are not related to each other.
On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote: > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: > > When reading md code, I find md-bitmap.c copies __clear_page_buffers from > > buffer.c, and after more search, seems there are some places in fs could > > use this function directly. So this patchset tries to export the function > > and use it to cleanup code. > > OK, I see why you did this, but there are a couple of problems with it. > > One is just a sequencing problem; between exporting __clear_page_buffers() > and removing it from the md code, the md code won't build. > > More seriously, most of this code has nothing to do with buffers. It > uses page->private for its own purposes. > > What I would do instead is add: > > clear_page_private(struct page *page) > { > ClearPagePrivate(page); > set_page_private(page, 0); > put_page(page); > } > > to include/linux/mm.h, then convert all callers of __clear_page_buffers() > to call that instead. While I think this is the right direction, I don't like the lack of symmetry between set_page_private() and clear_page_private() this creates. i.e. set_page_private() just assigned page->private, while clear_page_private clears both a page flag and page->private, and it also drops a page reference, too. Anyone expecting to use set/clear_page_private as a matched pair (as the names suggest they are) is in for a horrible surprise... This is a public service message brought to you by the Department of We Really Suck At API Design. Cheers, Dave.
On Mon, Apr 20, 2020 at 09:20:46AM +1000, Dave Chinner wrote: > On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote: > > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: > > > When reading md code, I find md-bitmap.c copies __clear_page_buffers from > > > buffer.c, and after more search, seems there are some places in fs could > > > use this function directly. So this patchset tries to export the function > > > and use it to cleanup code. > > > > OK, I see why you did this, but there are a couple of problems with it. > > > > One is just a sequencing problem; between exporting __clear_page_buffers() > > and removing it from the md code, the md code won't build. > > > > More seriously, most of this code has nothing to do with buffers. It > > uses page->private for its own purposes. > > > > What I would do instead is add: > > > > clear_page_private(struct page *page) > > { > > ClearPagePrivate(page); > > set_page_private(page, 0); > > put_page(page); > > } > > > > to include/linux/mm.h, then convert all callers of __clear_page_buffers() > > to call that instead. > > While I think this is the right direction, I don't like the lack of > symmetry between set_page_private() and clear_page_private() this > creates. i.e. set_page_private() just assigned page->private, while > clear_page_private clears both a page flag and page->private, and it > also drops a page reference, too. > > Anyone expecting to use set/clear_page_private as a matched pair (as > the names suggest they are) is in for a horrible surprise... > > This is a public service message brought to you by the Department > of We Really Suck At API Design. Oh, blast. I hadn't noticed that. And we're horribly inconsistent with how we use set_page_private() too -- rb_alloc_aux_page() doesn't increment the page's refcount, for example. So, new (pair of) names: set_fs_page_private() clear_fs_page_private() since it really seems like it's only page cache pages which need to follow the rules about setting PagePrivate and incrementing the refcount. Also, I think I'd like to see them take/return a void *: void *set_fs_page_private(struct page *page, void *data) { get_page(page); set_page_private(page, (unsigned long)data); SetPagePrivate(page); return data; } void *clear_fs_page_private(struct page *page) { void *data = (void *)page_private(page); if (!PagePrivate(page)) return NULL; ClearPagePrivate(page); set_page_private(page, 0); put_page(page); return data; } That makes iomap simpler: static void iomap_page_release(struct page *page) { - struct iomap_page *iop = to_iomap_page(page); + struct iomap_page *iop = clear_fs_page_private(page); if (!iop) return; WARN_ON_ONCE(atomic_read(&iop->read_count)); WARN_ON_ONCE(atomic_read(&iop->write_count)); - ClearPagePrivate(page); - set_page_private(page, 0); - put_page(page); kfree(iop); }
On 20.04.20 02:30, Matthew Wilcox wrote: > On Mon, Apr 20, 2020 at 09:20:46AM +1000, Dave Chinner wrote: >> On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote: >>> On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: >>>> When reading md code, I find md-bitmap.c copies __clear_page_buffers from >>>> buffer.c, and after more search, seems there are some places in fs could >>>> use this function directly. So this patchset tries to export the function >>>> and use it to cleanup code. >>> OK, I see why you did this, but there are a couple of problems with it. >>> >>> One is just a sequencing problem; between exporting __clear_page_buffers() >>> and removing it from the md code, the md code won't build. >>> >>> More seriously, most of this code has nothing to do with buffers. It >>> uses page->private for its own purposes. >>> >>> What I would do instead is add: >>> >>> clear_page_private(struct page *page) >>> { >>> ClearPagePrivate(page); >>> set_page_private(page, 0); >>> put_page(page); >>> } >>> >>> to include/linux/mm.h, then convert all callers of __clear_page_buffers() >>> to call that instead. >> While I think this is the right direction, I don't like the lack of >> symmetry between set_page_private() and clear_page_private() this >> creates. i.e. set_page_private() just assigned page->private, while >> clear_page_private clears both a page flag and page->private, and it >> also drops a page reference, too. >> >> Anyone expecting to use set/clear_page_private as a matched pair (as >> the names suggest they are) is in for a horrible surprise... Dave, thanks for the valuable suggestion! >> This is a public service message brought to you by the Department >> of We Really Suck At API Design. > Oh, blast. I hadn't noticed that. And we're horribly inconsistent > with how we use set_page_private() too -- rb_alloc_aux_page() doesn't > increment the page's refcount, for example. > > So, new (pair of) names: > > set_fs_page_private() > clear_fs_page_private() Hmm, maybe it is better to keep the original name (set/clear_page_private). Because, 1. it would be weird for other subsystems (not belong to fs scope) to call the function which is supposed to be used in fs, though we can add a wrapper for other users out of fs. 2. no function in mm.h is named like *fs*. > since it really seems like it's only page cache pages which need to > follow the rules about setting PagePrivate and incrementing the refcount. > Also, I think I'd like to see them take/return a void *: > > void *set_fs_page_private(struct page *page, void *data) > { > get_page(page); > set_page_private(page, (unsigned long)data); > SetPagePrivate(page); > return data; > } Seems some functions could probably use the above helper, such as iomap_page_create, iomap_migrate_page, get_page_bootmem and f2fs_set_page_private etc. > void *clear_fs_page_private(struct page *page) > { > void *data = (void *)page_private(page); > > if (!PagePrivate(page)) > return NULL; > ClearPagePrivate(page); > set_page_private(page, 0); > put_page(page); > return data; > } > > That makes iomap simpler: > > static void > iomap_page_release(struct page *page) > { > - struct iomap_page *iop = to_iomap_page(page); > + struct iomap_page *iop = clear_fs_page_private(page); > > if (!iop) > return; > WARN_ON_ONCE(atomic_read(&iop->read_count)); > WARN_ON_ONCE(atomic_read(&iop->write_count)); > - ClearPagePrivate(page); > - set_page_private(page, 0); > - put_page(page); > kfree(iop); > } > Really appreciate for your input though the thing is a little beyond my original intention ;-), will try to send a new version after reading more fs code. Best Regards, Guoqing
On Mon, Apr 20, 2020 at 11:14:35PM +0200, Guoqing Jiang wrote: > On 20.04.20 02:30, Matthew Wilcox wrote: > > On Mon, Apr 20, 2020 at 09:20:46AM +1000, Dave Chinner wrote: > > > Anyone expecting to use set/clear_page_private as a matched pair (as > > > the names suggest they are) is in for a horrible surprise... > > Dave, thanks for the valuable suggestion! > > > Oh, blast. I hadn't noticed that. And we're horribly inconsistent > > with how we use set_page_private() too -- rb_alloc_aux_page() doesn't > > increment the page's refcount, for example. > > > > So, new (pair of) names: > > > > set_fs_page_private() > > clear_fs_page_private() > > Hmm, maybe it is better to keep the original name (set/clear_page_private). No. Changing the semantics of set_page_private() without changing the function signature is bad because it makes patches silently break when applied to trees on either side of the change. So we need a new name. > 1. it would be weird for other subsystems (not belong to fs scope) to call > the > function which is supposed to be used in fs, though we can add a wrapper > for other users out of fs. > > 2. no function in mm.h is named like *fs*. perhaps it should be in pagemap.h since it's for pagecache pages. > > since it really seems like it's only page cache pages which need to > > follow the rules about setting PagePrivate and incrementing the refcount. > > Also, I think I'd like to see them take/return a void *: > > > > void *set_fs_page_private(struct page *page, void *data) > > { > > get_page(page); > > set_page_private(page, (unsigned long)data); > > SetPagePrivate(page); > > return data; > > } > > Seems some functions could probably use the above helper, such as > iomap_page_create, iomap_migrate_page, get_page_bootmem and > f2fs_set_page_private etc. Yes. > Really appreciate for your input though the thing is a little beyond my > original intention ;-), will try to send a new version after reading more > fs code. That's kind of the way things go ... you start pulling on one thread and all of a sudden, you're weaving a new coat ;-)
On Sun, Apr 19, 2020 at 05:30:25PM -0700, Matthew Wilcox wrote: > On Mon, Apr 20, 2020 at 09:20:46AM +1000, Dave Chinner wrote: > > On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote: > > > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote: > > > > When reading md code, I find md-bitmap.c copies __clear_page_buffers from > > > > buffer.c, and after more search, seems there are some places in fs could > > > > use this function directly. So this patchset tries to export the function > > > > and use it to cleanup code. > > > > > > OK, I see why you did this, but there are a couple of problems with it. > > > > > > One is just a sequencing problem; between exporting __clear_page_buffers() > > > and removing it from the md code, the md code won't build. > > > > > > More seriously, most of this code has nothing to do with buffers. It > > > uses page->private for its own purposes. > > > > > > What I would do instead is add: > > > > > > clear_page_private(struct page *page) > > > { > > > ClearPagePrivate(page); > > > set_page_private(page, 0); > > > put_page(page); > > > } > > > > > > to include/linux/mm.h, then convert all callers of __clear_page_buffers() > > > to call that instead. > > > > While I think this is the right direction, I don't like the lack of > > symmetry between set_page_private() and clear_page_private() this > > creates. i.e. set_page_private() just assigned page->private, while > > clear_page_private clears both a page flag and page->private, and it > > also drops a page reference, too. > > > > Anyone expecting to use set/clear_page_private as a matched pair (as > > the names suggest they are) is in for a horrible surprise... > > > > This is a public service message brought to you by the Department > > of We Really Suck At API Design. > > Oh, blast. I hadn't noticed that. And we're horribly inconsistent > with how we use set_page_private() too -- rb_alloc_aux_page() doesn't > increment the page's refcount, for example. > > So, new (pair of) names: > > set_fs_page_private() > clear_fs_page_private() Except set_fs() is already used for something else entirely, so now we have the same prefix having completely different meanings. Naming is hard. Realistically, we want these interfaces for use cases where we have an external object we store in page->private, right? That's an unsigned long, not a pointer, so we have to cast page->private for this sort of use. So, yes, I'd definitely like to see: > since it really seems like it's only page cache pages which need to > follow the rules about setting PagePrivate and incrementing the refcount. > Also, I think I'd like to see them take/return a void *: This sort of API cleanup. > void *set_fs_page_private(struct page *page, void *data) > { > get_page(page); > set_page_private(page, (unsigned long)data); > SetPagePrivate(page); > return data; > } > > void *clear_fs_page_private(struct page *page) > { > void *data = (void *)page_private(page); > > if (!PagePrivate(page)) > return NULL; > ClearPagePrivate(page); > set_page_private(page, 0); > put_page(page); > return data; > } I think we need a page_private() variant that returns a void * rather than having to cast it everywhere we directly access the private pointer. i.e. The API becomes: page_get_private_ptr() - replaces page_private() page_set_private_ptr() - replaces set_page_private() page_clear_private_ptr() - replaces clear_page_private() > That makes iomap simpler: > > static void > iomap_page_release(struct page *page) > { > - struct iomap_page *iop = to_iomap_page(page); > + struct iomap_page *iop = clear_fs_page_private(page); > > if (!iop) > return; > WARN_ON_ONCE(atomic_read(&iop->read_count)); > WARN_ON_ONCE(atomic_read(&iop->write_count)); > - ClearPagePrivate(page); > - set_page_private(page, 0); > - put_page(page); > kfree(iop); > } *nod* Much nicers :) Cheers, Dave.