* Filesystem crashes due to pages without buffers @ 2018-01-03 10:04 ` Jan Kara 0 siblings, 0 replies; 16+ messages in thread From: Jan Kara @ 2018-01-03 10:04 UTC (permalink / raw) To: linux-mm; +Cc: linux-fsdevel, linux-xfs, linux-ext4, Dan Williams Hello, Over the years I have seen so far unexplained crashed in filesystem's (ext4, xfs) writeback path due to dirty pages without buffers attached to them (see [1] and [2] for relatively recent reports). This was confusing as reclaim takes care not to strip buffers from a dirty page and both filesystems do add buffers to a page when it is first written to - in ->page_mkwrite() and ->write_begin callbacks. Recently I have come across a code path that is probably leading to this inconsistent state and I'd like to discuss how to best fix the problem because it's not obvious to me. Consider the following race: CPU1 CPU2 addr = mmap(file1, MAP_SHARED, ...); fd2 = open(file2, O_DIRECT | O_RDONLY); read(fd2, addr, len) do_direct_IO() page = dio_get_page() dio_refill_pages() iov_iter_get_pages() get_user_pages_fast() - page fault ->page_mkwrite() block_page_mkwrite() lock_page(page); - attaches buffers to page - makes sure blocks are allocated set_page_dirty(page) - install writeable PTE unlock_page(page); submit_page_section(page) - submits bio with 'page' as a buffer kswapd reclaims pages: ... shrink_page_list() trylock_page(page) - this is the page CPU1 has just faulted in try_to_unmap(page) pageout(page); clear_page_dirty_for_io(page); ->writepage() - let's assume page got written out fast enough, alternatively we could get to the same path as soon as the page IO completes if (page_has_private(page)) { try_to_release_page(page) - reclaims buffers from the page __remove_mapping(page) - fails as DIO code still holds page reference ... eventually read completes dio_bio_complete(bio) set_page_dirty_lock(page) Bummer, we've just marked the page as dirty without having buffers. Eventually writeback will find it and filesystem will complain... Am I missing something? The problem here is that filesystems fundamentally assume that a page can be written to only between ->write_begin - ->write_end (in this interval the page is locked), or between ->page_mkwrite - ->writepage and above is an example where this does not hold because when a page reference is acquired through get_user_pages(), page can get written to by the holder of the reference and dirtied even after it has been unmapped from page tables and ->writepage has been called. This is not only a cosmetic issue leading to assertion failure but it can also lead to data loss, data corruption, or other unpleasant surprises as filesystems assume page contents cannot be modified until either ->write_begin() or ->page_mkwrite gets called and those calls are serialized by proper locking with problematic operations such as hole punching etc. I'm not sure how to fix this problem. We could 'simulate' a writeable page fault in set_page_dirty_lock(). It is a bit ugly since we don't have a virtual address of the fault, don't hold mmap_sem, etc., possibly expensive, but it would make filesystems happy. Data stored by GUP user (e.g. read by DIO in the above case) could still get lost if someone e.g. punched hole under the buffer or otherwise messed with the underlying storage of the page while DIO was running but arguably users could expect such outcome. Another possible solution would be to make sure page is writeably mapped until GUP user drops its reference. That would be arguably cleaner but probably that would mean we have to track number of writeable GUP page references separately (no space space in struct page is a problem here) and block page_mkclean() until they are dropped. Also for long term GUP users like Infiniband or V4L we'd have to come up with some solution as we should not block page_mkclean() for so long. As a side note DAX needs some solution for GUP users as well. The problems are similar there in nature, just much easier to hit. So at least a solution for long-term GUP users can (and I strongly believe should) be shared between standard and DAX paths. Anybody has other ideas how to fix the problem or opinions on which solution would be better to use or some complications I have missed? Honza [1] https://www.spinics.net/lists/linux-xfs/msg10090.html [2] https://www.spinics.net/lists/linux-ext4/msg54377.html -- Jan Kara <jack@suse.com> SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Filesystem crashes due to pages without buffers @ 2018-01-03 10:04 ` Jan Kara 0 siblings, 0 replies; 16+ messages in thread From: Jan Kara @ 2018-01-03 10:04 UTC (permalink / raw) To: linux-mm; +Cc: linux-fsdevel, linux-xfs, linux-ext4, Dan Williams Hello, Over the years I have seen so far unexplained crashed in filesystem's (ext4, xfs) writeback path due to dirty pages without buffers attached to them (see [1] and [2] for relatively recent reports). This was confusing as reclaim takes care not to strip buffers from a dirty page and both filesystems do add buffers to a page when it is first written to - in ->page_mkwrite() and ->write_begin callbacks. Recently I have come across a code path that is probably leading to this inconsistent state and I'd like to discuss how to best fix the problem because it's not obvious to me. Consider the following race: CPU1 CPU2 addr = mmap(file1, MAP_SHARED, ...); fd2 = open(file2, O_DIRECT | O_RDONLY); read(fd2, addr, len) do_direct_IO() page = dio_get_page() dio_refill_pages() iov_iter_get_pages() get_user_pages_fast() - page fault ->page_mkwrite() block_page_mkwrite() lock_page(page); - attaches buffers to page - makes sure blocks are allocated set_page_dirty(page) - install writeable PTE unlock_page(page); submit_page_section(page) - submits bio with 'page' as a buffer kswapd reclaims pages: ... shrink_page_list() trylock_page(page) - this is the page CPU1 has just faulted in try_to_unmap(page) pageout(page); clear_page_dirty_for_io(page); ->writepage() - let's assume page got written out fast enough, alternatively we could get to the same path as soon as the page IO completes if (page_has_private(page)) { try_to_release_page(page) - reclaims buffers from the page __remove_mapping(page) - fails as DIO code still holds page reference ... eventually read completes dio_bio_complete(bio) set_page_dirty_lock(page) Bummer, we've just marked the page as dirty without having buffers. Eventually writeback will find it and filesystem will complain... Am I missing something? The problem here is that filesystems fundamentally assume that a page can be written to only between ->write_begin - ->write_end (in this interval the page is locked), or between ->page_mkwrite - ->writepage and above is an example where this does not hold because when a page reference is acquired through get_user_pages(), page can get written to by the holder of the reference and dirtied even after it has been unmapped from page tables and ->writepage has been called. This is not only a cosmetic issue leading to assertion failure but it can also lead to data loss, data corruption, or other unpleasant surprises as filesystems assume page contents cannot be modified until either ->write_begin() or ->page_mkwrite gets called and those calls are serialized by proper locking with problematic operations such as hole punching etc. I'm not sure how to fix this problem. We could 'simulate' a writeable page fault in set_page_dirty_lock(). It is a bit ugly since we don't have a virtual address of the fault, don't hold mmap_sem, etc., possibly expensive, but it would make filesystems happy. Data stored by GUP user (e.g. read by DIO in the above case) could still get lost if someone e.g. punched hole under the buffer or otherwise messed with the underlying storage of the page while DIO was running but arguably users could expect such outcome. Another possible solution would be to make sure page is writeably mapped until GUP user drops its reference. That would be arguably cleaner but probably that would mean we have to track number of writeable GUP page references separately (no space space in struct page is a problem here) and block page_mkclean() until they are dropped. Also for long term GUP users like Infiniband or V4L we'd have to come up with some solution as we should not block page_mkclean() for so long. As a side note DAX needs some solution for GUP users as well. The problems are similar there in nature, just much easier to hit. So at least a solution for long-term GUP users can (and I strongly believe should) be shared between standard and DAX paths. Anybody has other ideas how to fix the problem or opinions on which solution would be better to use or some complications I have missed? Honza [1] https://www.spinics.net/lists/linux-xfs/msg10090.html [2] https://www.spinics.net/lists/linux-ext4/msg54377.html -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-03 10:04 ` Jan Kara @ 2018-01-04 4:56 ` Dan Williams -1 siblings, 0 replies; 16+ messages in thread From: Dan Williams @ 2018-01-04 4:56 UTC (permalink / raw) To: Jan Kara; +Cc: Linux MM, linux-fsdevel, linux-xfs, linux-ext4 On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: > Hello, > > Over the years I have seen so far unexplained crashed in filesystem's > (ext4, xfs) writeback path due to dirty pages without buffers attached to > them (see [1] and [2] for relatively recent reports). This was confusing as > reclaim takes care not to strip buffers from a dirty page and both > filesystems do add buffers to a page when it is first written to - in > ->page_mkwrite() and ->write_begin callbacks. > > Recently I have come across a code path that is probably leading to this > inconsistent state and I'd like to discuss how to best fix the problem > because it's not obvious to me. Consider the following race: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > page = dio_get_page() > dio_refill_pages() > iov_iter_get_pages() > get_user_pages_fast() > - page fault > ->page_mkwrite() > block_page_mkwrite() > lock_page(page); > - attaches buffers to page > - makes sure blocks are allocated > set_page_dirty(page) > - install writeable PTE > unlock_page(page); > submit_page_section(page) > - submits bio with 'page' as a buffer > kswapd reclaims pages: > ... > shrink_page_list() > trylock_page(page) - this is the > page CPU1 has just faulted in > try_to_unmap(page) > pageout(page); > clear_page_dirty_for_io(page); > ->writepage() > - let's assume page got written > out fast enough, alternatively > we could get to the same path as > soon as the page IO completes > if (page_has_private(page)) { > try_to_release_page(page) > - reclaims buffers from the > page > __remove_mapping(page) > - fails as DIO code still > holds page reference > ... > > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > Bummer, we've just marked the page as dirty without having buffers. > Eventually writeback will find it and filesystem will complain... > > Am I missing something? > > The problem here is that filesystems fundamentally assume that a page can > be written to only between ->write_begin - ->write_end (in this interval > the page is locked), or between ->page_mkwrite - ->writepage and above is > an example where this does not hold because when a page reference is > acquired through get_user_pages(), page can get written to by the holder of > the reference and dirtied even after it has been unmapped from page tables > and ->writepage has been called. This is not only a cosmetic issue leading > to assertion failure but it can also lead to data loss, data corruption, or > other unpleasant surprises as filesystems assume page contents cannot be > modified until either ->write_begin() or ->page_mkwrite gets called and > those calls are serialized by proper locking with problematic operations > such as hole punching etc. > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > virtual address of the fault, don't hold mmap_sem, etc., possibly > expensive, but it would make filesystems happy. Data stored by GUP user > (e.g. read by DIO in the above case) could still get lost if someone e.g. > punched hole under the buffer or otherwise messed with the underlying > storage of the page while DIO was running but arguably users could expect > such outcome. > > Another possible solution would be to make sure page is writeably mapped > until GUP user drops its reference. That would be arguably cleaner but > probably that would mean we have to track number of writeable GUP page > references separately (no space space in struct page is a problem here) and > block page_mkclean() until they are dropped. Also for long term GUP users > like Infiniband or V4L we'd have to come up with some solution as we should > not block page_mkclean() for so long. Do we need to block page_mkclean, or could we defer buffer reclaiming to the last put of the page? I think once we have the "register memory with lease" mechanism for Infiniband we could expand it to the page cache case. The problem is the regression this would cause with userspace that expects it can maintain file backed memory registrations indefinitely. What are the implications of holding off page_mkclean or release buffers indefinitely? Is an indefinite / interruptible sleep waiting for the 'put' event of a get_user_pages() page unacceptable? The current case that the file contents will not be coherent with respect to in-flight RDMA, perhaps waiting for that to complete is better than cleaning buffers from the page prematurely. Yes, I have more questions than proposals. > > As a side note DAX needs some solution for GUP users as well. The problems > are similar there in nature, just much easier to hit. So at least a > solution for long-term GUP users can (and I strongly believe should) be > shared between standard and DAX paths. In the DAX case we rely on the fact that when the page goes idle we only need to worry about the filesytem block map changing, the page won't get reallocated somewhere else. We can't use page idle as an event in this case, however, if the page reference count is one then the DIO code can know that it has the page exclusively, so maybe DAX and non-DAX can share the page count == 1 event notification. However there's still the matter of how to callback into the filesystem. The DAX case is currently using a pgmap_radix lookup at put_page() time to determine when to wakeup waiters. I think this should move over to a new address_space_operation. That would help with reusing some of the DAX case machinery for this case, but the DIO code would need a special case put_page that checks for count == 1 and synchronization to hold off the DIO submission path to prevent new page elevations. ...are you sure this is still similar enough to the DAX case that they can reuse much of the same machinery? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers @ 2018-01-04 4:56 ` Dan Williams 0 siblings, 0 replies; 16+ messages in thread From: Dan Williams @ 2018-01-04 4:56 UTC (permalink / raw) To: Jan Kara; +Cc: Linux MM, linux-fsdevel, linux-xfs, linux-ext4 On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: > Hello, > > Over the years I have seen so far unexplained crashed in filesystem's > (ext4, xfs) writeback path due to dirty pages without buffers attached to > them (see [1] and [2] for relatively recent reports). This was confusing as > reclaim takes care not to strip buffers from a dirty page and both > filesystems do add buffers to a page when it is first written to - in > ->page_mkwrite() and ->write_begin callbacks. > > Recently I have come across a code path that is probably leading to this > inconsistent state and I'd like to discuss how to best fix the problem > because it's not obvious to me. Consider the following race: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > page = dio_get_page() > dio_refill_pages() > iov_iter_get_pages() > get_user_pages_fast() > - page fault > ->page_mkwrite() > block_page_mkwrite() > lock_page(page); > - attaches buffers to page > - makes sure blocks are allocated > set_page_dirty(page) > - install writeable PTE > unlock_page(page); > submit_page_section(page) > - submits bio with 'page' as a buffer > kswapd reclaims pages: > ... > shrink_page_list() > trylock_page(page) - this is the > page CPU1 has just faulted in > try_to_unmap(page) > pageout(page); > clear_page_dirty_for_io(page); > ->writepage() > - let's assume page got written > out fast enough, alternatively > we could get to the same path as > soon as the page IO completes > if (page_has_private(page)) { > try_to_release_page(page) > - reclaims buffers from the > page > __remove_mapping(page) > - fails as DIO code still > holds page reference > ... > > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > Bummer, we've just marked the page as dirty without having buffers. > Eventually writeback will find it and filesystem will complain... > > Am I missing something? > > The problem here is that filesystems fundamentally assume that a page can > be written to only between ->write_begin - ->write_end (in this interval > the page is locked), or between ->page_mkwrite - ->writepage and above is > an example where this does not hold because when a page reference is > acquired through get_user_pages(), page can get written to by the holder of > the reference and dirtied even after it has been unmapped from page tables > and ->writepage has been called. This is not only a cosmetic issue leading > to assertion failure but it can also lead to data loss, data corruption, or > other unpleasant surprises as filesystems assume page contents cannot be > modified until either ->write_begin() or ->page_mkwrite gets called and > those calls are serialized by proper locking with problematic operations > such as hole punching etc. > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > virtual address of the fault, don't hold mmap_sem, etc., possibly > expensive, but it would make filesystems happy. Data stored by GUP user > (e.g. read by DIO in the above case) could still get lost if someone e.g. > punched hole under the buffer or otherwise messed with the underlying > storage of the page while DIO was running but arguably users could expect > such outcome. > > Another possible solution would be to make sure page is writeably mapped > until GUP user drops its reference. That would be arguably cleaner but > probably that would mean we have to track number of writeable GUP page > references separately (no space space in struct page is a problem here) and > block page_mkclean() until they are dropped. Also for long term GUP users > like Infiniband or V4L we'd have to come up with some solution as we should > not block page_mkclean() for so long. Do we need to block page_mkclean, or could we defer buffer reclaiming to the last put of the page? I think once we have the "register memory with lease" mechanism for Infiniband we could expand it to the page cache case. The problem is the regression this would cause with userspace that expects it can maintain file backed memory registrations indefinitely. What are the implications of holding off page_mkclean or release buffers indefinitely? Is an indefinite / interruptible sleep waiting for the 'put' event of a get_user_pages() page unacceptable? The current case that the file contents will not be coherent with respect to in-flight RDMA, perhaps waiting for that to complete is better than cleaning buffers from the page prematurely. Yes, I have more questions than proposals. > > As a side note DAX needs some solution for GUP users as well. The problems > are similar there in nature, just much easier to hit. So at least a > solution for long-term GUP users can (and I strongly believe should) be > shared between standard and DAX paths. In the DAX case we rely on the fact that when the page goes idle we only need to worry about the filesytem block map changing, the page won't get reallocated somewhere else. We can't use page idle as an event in this case, however, if the page reference count is one then the DIO code can know that it has the page exclusively, so maybe DAX and non-DAX can share the page count == 1 event notification. However there's still the matter of how to callback into the filesystem. The DAX case is currently using a pgmap_radix lookup at put_page() time to determine when to wakeup waiters. I think this should move over to a new address_space_operation. That would help with reusing some of the DAX case machinery for this case, but the DIO code would need a special case put_page that checks for count == 1 and synchronization to hold off the DIO submission path to prevent new page elevations. ...are you sure this is still similar enough to the DAX case that they can reuse much of the same machinery? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-04 4:56 ` Dan Williams @ 2018-01-04 11:33 ` Jan Kara -1 siblings, 0 replies; 16+ messages in thread From: Jan Kara @ 2018-01-04 11:33 UTC (permalink / raw) To: Dan Williams; +Cc: Jan Kara, Linux MM, linux-fsdevel, linux-xfs, linux-ext4 On Wed 03-01-18 20:56:32, Dan Williams wrote: > On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: > > Hello, > > > > Over the years I have seen so far unexplained crashed in filesystem's > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > them (see [1] and [2] for relatively recent reports). This was confusing as > > reclaim takes care not to strip buffers from a dirty page and both > > filesystems do add buffers to a page when it is first written to - in > > ->page_mkwrite() and ->write_begin callbacks. > > > > Recently I have come across a code path that is probably leading to this > > inconsistent state and I'd like to discuss how to best fix the problem > > because it's not obvious to me. Consider the following race: > > > > CPU1 CPU2 > > > > addr = mmap(file1, MAP_SHARED, ...); > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > read(fd2, addr, len) > > do_direct_IO() > > page = dio_get_page() > > dio_refill_pages() > > iov_iter_get_pages() > > get_user_pages_fast() > > - page fault > > ->page_mkwrite() > > block_page_mkwrite() > > lock_page(page); > > - attaches buffers to page > > - makes sure blocks are allocated > > set_page_dirty(page) > > - install writeable PTE > > unlock_page(page); > > submit_page_section(page) > > - submits bio with 'page' as a buffer > > kswapd reclaims pages: > > ... > > shrink_page_list() > > trylock_page(page) - this is the > > page CPU1 has just faulted in > > try_to_unmap(page) > > pageout(page); > > clear_page_dirty_for_io(page); > > ->writepage() > > - let's assume page got written > > out fast enough, alternatively > > we could get to the same path as > > soon as the page IO completes > > if (page_has_private(page)) { > > try_to_release_page(page) > > - reclaims buffers from the > > page > > __remove_mapping(page) > > - fails as DIO code still > > holds page reference > > ... > > > > eventually read completes > > dio_bio_complete(bio) > > set_page_dirty_lock(page) > > Bummer, we've just marked the page as dirty without having buffers. > > Eventually writeback will find it and filesystem will complain... > > > > Am I missing something? > > > > The problem here is that filesystems fundamentally assume that a page can > > be written to only between ->write_begin - ->write_end (in this interval > > the page is locked), or between ->page_mkwrite - ->writepage and above is > > an example where this does not hold because when a page reference is > > acquired through get_user_pages(), page can get written to by the holder of > > the reference and dirtied even after it has been unmapped from page tables > > and ->writepage has been called. This is not only a cosmetic issue leading > > to assertion failure but it can also lead to data loss, data corruption, or > > other unpleasant surprises as filesystems assume page contents cannot be > > modified until either ->write_begin() or ->page_mkwrite gets called and > > those calls are serialized by proper locking with problematic operations > > such as hole punching etc. > > > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > > virtual address of the fault, don't hold mmap_sem, etc., possibly > > expensive, but it would make filesystems happy. Data stored by GUP user > > (e.g. read by DIO in the above case) could still get lost if someone e.g. > > punched hole under the buffer or otherwise messed with the underlying > > storage of the page while DIO was running but arguably users could expect > > such outcome. > > > > Another possible solution would be to make sure page is writeably mapped > > until GUP user drops its reference. That would be arguably cleaner but > > probably that would mean we have to track number of writeable GUP page > > references separately (no space space in struct page is a problem here) and > > block page_mkclean() until they are dropped. Also for long term GUP users > > like Infiniband or V4L we'd have to come up with some solution as we should > > not block page_mkclean() for so long. > > Do we need to block page_mkclean, or could we defer buffer reclaiming > to the last put of the page? As I wrote to Dave the problem is no so much with reclaiming of buffers but with the fact filesystems don't expect page can be dirtied after page_mkclean() is finished. > I think once we have the "register memory with lease" mechanism for > Infiniband we could expand it to the page cache case. The problem is > the regression this would cause with userspace that expects it can > maintain file backed memory registrations indefinitely. > > What are the implications of holding off page_mkclean or release > buffers indefinitely? Bad. You cannot write the page to disk until page_mkclean() finishes as page_mkclean() is part of clear_page_dirty_for_io(). And we really do need that functionality there e.g. to make sure tail of the last page in the file is properly zeroed out, storage with DIF/DIX can compute checksum of the data safely before submitting it to the device etc. > Is an indefinite / interruptible sleep waiting for the 'put' event of > a get_user_pages() page unacceptable? The current case that the file > contents will not be coherent with respect to in-flight RDMA, perhaps > waiting for that to complete is better than cleaning buffers from the > page prematurely. Yeah, indefinite sleep is really a no-go. > > As a side note DAX needs some solution for GUP users as well. The problems > > are similar there in nature, just much easier to hit. So at least a > > solution for long-term GUP users can (and I strongly believe should) be > > shared between standard and DAX paths. > > In the DAX case we rely on the fact that when the page goes idle we > only need to worry about the filesytem block map changing, the page > won't get reallocated somewhere else. We can't use page idle as an > event in this case, however, if the page reference count is one then > the DIO code can know that it has the page exclusively, so maybe DAX > and non-DAX can share the page count == 1 event notification. The races I describe do not need to involve truncate / hole punching. It is just enough to race with page writeback. So page references are of no use here. We would have to specifically track number of references acquired by GUP or something like that. So what I wanted to share with DAX is the long-term pin handling, the rest is unclear for now. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers @ 2018-01-04 11:33 ` Jan Kara 0 siblings, 0 replies; 16+ messages in thread From: Jan Kara @ 2018-01-04 11:33 UTC (permalink / raw) To: Dan Williams; +Cc: Jan Kara, Linux MM, linux-fsdevel, linux-xfs, linux-ext4 On Wed 03-01-18 20:56:32, Dan Williams wrote: > On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: > > Hello, > > > > Over the years I have seen so far unexplained crashed in filesystem's > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > them (see [1] and [2] for relatively recent reports). This was confusing as > > reclaim takes care not to strip buffers from a dirty page and both > > filesystems do add buffers to a page when it is first written to - in > > ->page_mkwrite() and ->write_begin callbacks. > > > > Recently I have come across a code path that is probably leading to this > > inconsistent state and I'd like to discuss how to best fix the problem > > because it's not obvious to me. Consider the following race: > > > > CPU1 CPU2 > > > > addr = mmap(file1, MAP_SHARED, ...); > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > read(fd2, addr, len) > > do_direct_IO() > > page = dio_get_page() > > dio_refill_pages() > > iov_iter_get_pages() > > get_user_pages_fast() > > - page fault > > ->page_mkwrite() > > block_page_mkwrite() > > lock_page(page); > > - attaches buffers to page > > - makes sure blocks are allocated > > set_page_dirty(page) > > - install writeable PTE > > unlock_page(page); > > submit_page_section(page) > > - submits bio with 'page' as a buffer > > kswapd reclaims pages: > > ... > > shrink_page_list() > > trylock_page(page) - this is the > > page CPU1 has just faulted in > > try_to_unmap(page) > > pageout(page); > > clear_page_dirty_for_io(page); > > ->writepage() > > - let's assume page got written > > out fast enough, alternatively > > we could get to the same path as > > soon as the page IO completes > > if (page_has_private(page)) { > > try_to_release_page(page) > > - reclaims buffers from the > > page > > __remove_mapping(page) > > - fails as DIO code still > > holds page reference > > ... > > > > eventually read completes > > dio_bio_complete(bio) > > set_page_dirty_lock(page) > > Bummer, we've just marked the page as dirty without having buffers. > > Eventually writeback will find it and filesystem will complain... > > > > Am I missing something? > > > > The problem here is that filesystems fundamentally assume that a page can > > be written to only between ->write_begin - ->write_end (in this interval > > the page is locked), or between ->page_mkwrite - ->writepage and above is > > an example where this does not hold because when a page reference is > > acquired through get_user_pages(), page can get written to by the holder of > > the reference and dirtied even after it has been unmapped from page tables > > and ->writepage has been called. This is not only a cosmetic issue leading > > to assertion failure but it can also lead to data loss, data corruption, or > > other unpleasant surprises as filesystems assume page contents cannot be > > modified until either ->write_begin() or ->page_mkwrite gets called and > > those calls are serialized by proper locking with problematic operations > > such as hole punching etc. > > > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > > virtual address of the fault, don't hold mmap_sem, etc., possibly > > expensive, but it would make filesystems happy. Data stored by GUP user > > (e.g. read by DIO in the above case) could still get lost if someone e.g. > > punched hole under the buffer or otherwise messed with the underlying > > storage of the page while DIO was running but arguably users could expect > > such outcome. > > > > Another possible solution would be to make sure page is writeably mapped > > until GUP user drops its reference. That would be arguably cleaner but > > probably that would mean we have to track number of writeable GUP page > > references separately (no space space in struct page is a problem here) and > > block page_mkclean() until they are dropped. Also for long term GUP users > > like Infiniband or V4L we'd have to come up with some solution as we should > > not block page_mkclean() for so long. > > Do we need to block page_mkclean, or could we defer buffer reclaiming > to the last put of the page? As I wrote to Dave the problem is no so much with reclaiming of buffers but with the fact filesystems don't expect page can be dirtied after page_mkclean() is finished. > I think once we have the "register memory with lease" mechanism for > Infiniband we could expand it to the page cache case. The problem is > the regression this would cause with userspace that expects it can > maintain file backed memory registrations indefinitely. > > What are the implications of holding off page_mkclean or release > buffers indefinitely? Bad. You cannot write the page to disk until page_mkclean() finishes as page_mkclean() is part of clear_page_dirty_for_io(). And we really do need that functionality there e.g. to make sure tail of the last page in the file is properly zeroed out, storage with DIF/DIX can compute checksum of the data safely before submitting it to the device etc. > Is an indefinite / interruptible sleep waiting for the 'put' event of > a get_user_pages() page unacceptable? The current case that the file > contents will not be coherent with respect to in-flight RDMA, perhaps > waiting for that to complete is better than cleaning buffers from the > page prematurely. Yeah, indefinite sleep is really a no-go. > > As a side note DAX needs some solution for GUP users as well. The problems > > are similar there in nature, just much easier to hit. So at least a > > solution for long-term GUP users can (and I strongly believe should) be > > shared between standard and DAX paths. > > In the DAX case we rely on the fact that when the page goes idle we > only need to worry about the filesytem block map changing, the page > won't get reallocated somewhere else. We can't use page idle as an > event in this case, however, if the page reference count is one then > the DIO code can know that it has the page exclusively, so maybe DAX > and non-DAX can share the page count == 1 event notification. The races I describe do not need to involve truncate / hole punching. It is just enough to race with page writeback. So page references are of no use here. We would have to specifically track number of references acquired by GUP or something like that. So what I wanted to share with DAX is the long-term pin handling, the rest is unclear for now. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-04 11:33 ` Jan Kara (?) @ 2018-04-13 12:39 ` Gavin Guo 2018-04-13 12:58 ` Jan Kara -1 siblings, 1 reply; 16+ messages in thread From: Gavin Guo @ 2018-04-13 12:39 UTC (permalink / raw) To: Jan Kara; +Cc: Dan Williams, Linux MM, linux-fsdevel, linux-xfs, linux-ext4 Hi all, On Thu, Jan 4, 2018 at 7:33 PM, Jan Kara <jack@suse.cz> wrote: > > On Wed 03-01-18 20:56:32, Dan Williams wrote: > > On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: > > > Hello, > > > > > > Over the years I have seen so far unexplained crashed in filesystem's > > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > > them (see [1] and [2] for relatively recent reports). This was confusing as > > > reclaim takes care not to strip buffers from a dirty page and both > > > filesystems do add buffers to a page when it is first written to - in > > > ->page_mkwrite() and ->write_begin callbacks. > > > > > > Recently I have come across a code path that is probably leading to this > > > inconsistent state and I'd like to discuss how to best fix the problem > > > because it's not obvious to me. Consider the following race: > > > > > > CPU1 CPU2 > > > > > > addr = mmap(file1, MAP_SHARED, ...); > > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > > read(fd2, addr, len) > > > do_direct_IO() > > > page = dio_get_page() > > > dio_refill_pages() > > > iov_iter_get_pages() > > > get_user_pages_fast() > > > - page fault > > > ->page_mkwrite() > > > block_page_mkwrite() > > > lock_page(page); > > > - attaches buffers to page > > > - makes sure blocks are allocated > > > set_page_dirty(page) > > > - install writeable PTE > > > unlock_page(page); > > > submit_page_section(page) > > > - submits bio with 'page' as a buffer > > > kswapd reclaims pages: > > > ... > > > shrink_page_list() > > > trylock_page(page) - this is the > > > page CPU1 has just faulted in > > > try_to_unmap(page) > > > pageout(page); > > > clear_page_dirty_for_io(page); > > > ->writepage() > > > - let's assume page got written > > > out fast enough, alternatively > > > we could get to the same path as > > > soon as the page IO completes > > > if (page_has_private(page)) { > > > try_to_release_page(page) > > > - reclaims buffers from the > > > page > > > __remove_mapping(page) > > > - fails as DIO code still > > > holds page reference > > > ... > > > > > > eventually read completes > > > dio_bio_complete(bio) > > > set_page_dirty_lock(page) > > > Bummer, we've just marked the page as dirty without having buffers. > > > Eventually writeback will find it and filesystem will complain... > > > > > > Am I missing something? > > > > > > The problem here is that filesystems fundamentally assume that a page can > > > be written to only between ->write_begin - ->write_end (in this interval > > > the page is locked), or between ->page_mkwrite - ->writepage and above is > > > an example where this does not hold because when a page reference is > > > acquired through get_user_pages(), page can get written to by the holder of > > > the reference and dirtied even after it has been unmapped from page tables > > > and ->writepage has been called. This is not only a cosmetic issue leading > > > to assertion failure but it can also lead to data loss, data corruption, or > > > other unpleasant surprises as filesystems assume page contents cannot be > > > modified until either ->write_begin() or ->page_mkwrite gets called and > > > those calls are serialized by proper locking with problematic operations > > > such as hole punching etc. > > > > > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > > > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > > > virtual address of the fault, don't hold mmap_sem, etc., possibly > > > expensive, but it would make filesystems happy. Data stored by GUP user > > > (e.g. read by DIO in the above case) could still get lost if someone e.g. > > > punched hole under the buffer or otherwise messed with the underlying > > > storage of the page while DIO was running but arguably users could expect > > > such outcome. > > > > > > Another possible solution would be to make sure page is writeably mapped > > > until GUP user drops its reference. That would be arguably cleaner but > > > probably that would mean we have to track number of writeable GUP page > > > references separately (no space space in struct page is a problem here) and > > > block page_mkclean() until they are dropped. Also for long term GUP users > > > like Infiniband or V4L we'd have to come up with some solution as we should > > > not block page_mkclean() for so long. > > > > Do we need to block page_mkclean, or could we defer buffer reclaiming > > to the last put of the page? > > As I wrote to Dave the problem is no so much with reclaiming of buffers but > with the fact filesystems don't expect page can be dirtied after > page_mkclean() is finished. > > > I think once we have the "register memory with lease" mechanism for > > Infiniband we could expand it to the page cache case. The problem is > > the regression this would cause with userspace that expects it can > > maintain file backed memory registrations indefinitely. > > > > What are the implications of holding off page_mkclean or release > > buffers indefinitely? > > Bad. You cannot write the page to disk until page_mkclean() finishes as > page_mkclean() is part of clear_page_dirty_for_io(). And we really do need > that functionality there e.g. to make sure tail of the last page in the > file is properly zeroed out, storage with DIF/DIX can compute checksum of > the data safely before submitting it to the device etc. > > > Is an indefinite / interruptible sleep waiting for the 'put' event of > > a get_user_pages() page unacceptable? The current case that the file > > contents will not be coherent with respect to in-flight RDMA, perhaps > > waiting for that to complete is better than cleaning buffers from the > > page prematurely. > > Yeah, indefinite sleep is really a no-go. > > > > As a side note DAX needs some solution for GUP users as well. The problems > > > are similar there in nature, just much easier to hit. So at least a > > > solution for long-term GUP users can (and I strongly believe should) be > > > shared between standard and DAX paths. > > > > In the DAX case we rely on the fact that when the page goes idle we > > only need to worry about the filesytem block map changing, the page > > won't get reallocated somewhere else. We can't use page idle as an > > event in this case, however, if the page reference count is one then > > the DIO code can know that it has the page exclusively, so maybe DAX > > and non-DAX can share the page count == 1 event notification. > > The races I describe do not need to involve truncate / hole punching. It is > just enough to race with page writeback. So page references are of no use > here. We would have to specifically track number of references acquired by > GUP or something like that. So what I wanted to share with DAX is the > long-term pin handling, the rest is unclear for now. > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> The bug can be reliably reproduced in our platform with the current upstream kernel(80aa76bcd364 Merge tag 'xfs-4.17-merge-4' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux). I'm happy to help to test and debug. The error message is as following: kernel BUG at /home/gavin/work-kernel/fs/ext4/inode.c:2126! invalid opcode: 0000 [#1] SMP PTI Modules linked in: veth ipt_MASQUERADE nf_nat_masquerade_ipv4 nf_conntrack_netlink nfnetlink xfrm_user iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype xt_conntrack nf_nat nf_conntrack br_netfilter bridge stp llc overlay xt_multiport iptable_filter ip_tables x_tables cachefiles fscache esp6_offload esp6 esp4_offload esp4 xfrm_algo nls_iso8859_1 intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp ipmi_ssif coretemp kvm_intel nvidia_uvm(POE) mxm_wmi kvm joydev input_leds irqbypass intel_cstate intel_rapl_perf mei_me ipmi_si shpchp lpc_ich mei acpi_power_meter mac_hid wmi ib_iser rdma_cm iw_cm ib_cm ib_core iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi ipmi_devintf sunrpc ipmi_msghandler autofs4 btrfs zstd_compress raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 multipath linear i2c_algo_bit nvidia_drm(POE) ses crct10dif_pclmul crc32_pclmul nvidia_modeset(POE) ttm ghash_clmulni_intel enclosure hid_generic uas pcbc scsi_transport_sas drm_kms_helper usbhid aesni_intel hid aes_x86_64 usb_storage syscopyarea crypto_simd sysfillrect mlx5_core cryptd nvidia(POE) sysimgblt glue_helper ixgbe mlxfw megaraid_sas fb_sys_fops devlink dca ahci ptp drm libahci pps_core mdio CPU: 54 PID: 8938 Comm: kworker/u161:0 Tainted: P OE 4.16.0-999-generic #201804102200 Workqueue: writeback wb_workfn (flush-8:0) RIP: 0010:ext4_writepage+0x318/0x770 RSP: 0018:ffffb514e76cb7f8 EFLAGS: 00010246 RAX: 00500b4e8000026d RBX: 0000000000001000 RCX: ffff8c2cafba5000 RDX: ffff8bec4069a020 RSI: ffffb514e76cbc28 RDI: ffffe91efcc8bd80 RBP: ffffb514e76cb870 R08: 0000000000028115 R09: 00000000000280c0 R10: 0000000000000002 R11: ffff8c2dbffd4000 R12: ffffe91efcc8bd80 R13: ffff8bec40699ea8 R14: ffffb514e76cbc28 R15: ffffe91efcc8bd80 FS: 0000000000000000(0000) GS:ffff8becbfc80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007ffcc8f6f080 CR3: 0000004b9ce0a006 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: ? rmap_walk+0x41/0x60 ? page_mkclean+0x9f/0xb0 ? invalid_page_referenced_vma+0x80/0x80 __writepage+0x17/0x50 write_cache_pages+0x228/0x4a0 ? __wb_calc_thresh+0x140/0x140 generic_writepages+0x61/0xa0 ? _cond_resched+0x1a/0x50 ? write_cache_pages+0x396/0x4a0 ext4_writepages+0x1fc/0xe00 ? ext4_writepages+0x1fc/0xe00 ? generic_writepages+0x6d/0xa0 ? fprop_fraction_percpu+0x2f/0x80 do_writepages+0x1c/0x60 ? do_writepages+0x1c/0x60 __writeback_single_inode+0x45/0x320 writeback_sb_inodes+0x266/0x580 __writeback_inodes_wb+0x92/0xc0 wb_writeback+0x282/0x310 wb_workfn+0x1a3/0x440 ? wb_workfn+0x1a3/0x440 process_one_work+0x1db/0x3c0 worker_thread+0x4b/0x420 kthread+0x102/0x140 ? rescuer_thread+0x380/0x380 ? kthread_create_worker_on_cpu+0x70/0x70 ret_from_fork+0x35/0x40 Code: ff f6 c4 08 0f 85 58 ff ff ff e8 68 56 00 00 ba 00 10 00 00 31 f6 41 bd fb ff ff ff e8 a2 9e ff ff 4c 89 e7 e8 fa 39 ea ff eb 8a <0f> 0b c6 45 a8 00 e9 f0 fd ff ff 49 83 7c 24 10 00 0f 85 c7 03 RIP: ext4_writepage+0x318/0x770 RSP: ffffb514e76cb7f8 ---[ end trace 59d4e1a4b221404b ]--- ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-04-13 12:39 ` Gavin Guo @ 2018-04-13 12:58 ` Jan Kara 2018-04-16 9:20 ` Gavin Guo 0 siblings, 1 reply; 16+ messages in thread From: Jan Kara @ 2018-04-13 12:58 UTC (permalink / raw) To: Gavin Guo Cc: Jan Kara, Dan Williams, Linux MM, linux-fsdevel, linux-xfs, linux-ext4 Hi! On Fri 13-04-18 20:39:06, Gavin Guo wrote: > On Thu, Jan 4, 2018 at 7:33 PM, Jan Kara <jack@suse.cz> wrote: > > > > On Wed 03-01-18 20:56:32, Dan Williams wrote: > > > On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: > > > > Hello, > > > > > > > > Over the years I have seen so far unexplained crashed in filesystem's > > > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > > > them (see [1] and [2] for relatively recent reports). This was confusing as > > > > reclaim takes care not to strip buffers from a dirty page and both > > > > filesystems do add buffers to a page when it is first written to - in > > > > ->page_mkwrite() and ->write_begin callbacks. > > > > > > > > Recently I have come across a code path that is probably leading to this > > > > inconsistent state and I'd like to discuss how to best fix the problem > > > > because it's not obvious to me. Consider the following race: > > > > > > > > CPU1 CPU2 > > > > > > > > addr = mmap(file1, MAP_SHARED, ...); > > > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > > > read(fd2, addr, len) > > > > do_direct_IO() > > > > page = dio_get_page() > > > > dio_refill_pages() > > > > iov_iter_get_pages() > > > > get_user_pages_fast() > > > > - page fault > > > > ->page_mkwrite() > > > > block_page_mkwrite() > > > > lock_page(page); > > > > - attaches buffers to page > > > > - makes sure blocks are allocated > > > > set_page_dirty(page) > > > > - install writeable PTE > > > > unlock_page(page); > > > > submit_page_section(page) > > > > - submits bio with 'page' as a buffer > > > > kswapd reclaims pages: > > > > ... > > > > shrink_page_list() > > > > trylock_page(page) - this is the > > > > page CPU1 has just faulted in > > > > try_to_unmap(page) > > > > pageout(page); > > > > clear_page_dirty_for_io(page); > > > > ->writepage() > > > > - let's assume page got written > > > > out fast enough, alternatively > > > > we could get to the same path as > > > > soon as the page IO completes > > > > if (page_has_private(page)) { > > > > try_to_release_page(page) > > > > - reclaims buffers from the > > > > page > > > > __remove_mapping(page) > > > > - fails as DIO code still > > > > holds page reference > > > > ... > > > > > > > > eventually read completes > > > > dio_bio_complete(bio) > > > > set_page_dirty_lock(page) > > > > Bummer, we've just marked the page as dirty without having buffers. > > > > Eventually writeback will find it and filesystem will complain... > > > > > > > > Am I missing something? > > > > > > > > The problem here is that filesystems fundamentally assume that a page can > > > > be written to only between ->write_begin - ->write_end (in this interval > > > > the page is locked), or between ->page_mkwrite - ->writepage and above is > > > > an example where this does not hold because when a page reference is > > > > acquired through get_user_pages(), page can get written to by the holder of > > > > the reference and dirtied even after it has been unmapped from page tables > > > > and ->writepage has been called. This is not only a cosmetic issue leading > > > > to assertion failure but it can also lead to data loss, data corruption, or > > > > other unpleasant surprises as filesystems assume page contents cannot be > > > > modified until either ->write_begin() or ->page_mkwrite gets called and > > > > those calls are serialized by proper locking with problematic operations > > > > such as hole punching etc. > > > > > > > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > > > > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > > > > virtual address of the fault, don't hold mmap_sem, etc., possibly > > > > expensive, but it would make filesystems happy. Data stored by GUP user > > > > (e.g. read by DIO in the above case) could still get lost if someone e.g. > > > > punched hole under the buffer or otherwise messed with the underlying > > > > storage of the page while DIO was running but arguably users could expect > > > > such outcome. > > > > > > > > Another possible solution would be to make sure page is writeably mapped > > > > until GUP user drops its reference. That would be arguably cleaner but > > > > probably that would mean we have to track number of writeable GUP page > > > > references separately (no space space in struct page is a problem here) and > > > > block page_mkclean() until they are dropped. Also for long term GUP users > > > > like Infiniband or V4L we'd have to come up with some solution as we should > > > > not block page_mkclean() for so long. > > > > > > Do we need to block page_mkclean, or could we defer buffer reclaiming > > > to the last put of the page? > > > > As I wrote to Dave the problem is no so much with reclaiming of buffers but > > with the fact filesystems don't expect page can be dirtied after > > page_mkclean() is finished. > > > > > I think once we have the "register memory with lease" mechanism for > > > Infiniband we could expand it to the page cache case. The problem is > > > the regression this would cause with userspace that expects it can > > > maintain file backed memory registrations indefinitely. > > > > > > What are the implications of holding off page_mkclean or release > > > buffers indefinitely? > > > > Bad. You cannot write the page to disk until page_mkclean() finishes as > > page_mkclean() is part of clear_page_dirty_for_io(). And we really do need > > that functionality there e.g. to make sure tail of the last page in the > > file is properly zeroed out, storage with DIF/DIX can compute checksum of > > the data safely before submitting it to the device etc. > > > > > Is an indefinite / interruptible sleep waiting for the 'put' event of > > > a get_user_pages() page unacceptable? The current case that the file > > > contents will not be coherent with respect to in-flight RDMA, perhaps > > > waiting for that to complete is better than cleaning buffers from the > > > page prematurely. > > > > Yeah, indefinite sleep is really a no-go. > > > > > > As a side note DAX needs some solution for GUP users as well. The problems > > > > are similar there in nature, just much easier to hit. So at least a > > > > solution for long-term GUP users can (and I strongly believe should) be > > > > shared between standard and DAX paths. > > > > > > In the DAX case we rely on the fact that when the page goes idle we > > > only need to worry about the filesytem block map changing, the page > > > won't get reallocated somewhere else. We can't use page idle as an > > > event in this case, however, if the page reference count is one then > > > the DIO code can know that it has the page exclusively, so maybe DAX > > > and non-DAX can share the page count == 1 event notification. > > > > The races I describe do not need to involve truncate / hole punching. It is > > just enough to race with page writeback. So page references are of no use > > here. We would have to specifically track number of references acquired by > > GUP or something like that. So what I wanted to share with DAX is the > > long-term pin handling, the rest is unclear for now. > > > > Honza > > -- > > Jan Kara <jack@suse.com> > > SUSE Labs, CR > > > > -- > > To unsubscribe, send a message with 'unsubscribe linux-mm' in > > the body to majordomo@kvack.org. For more info on Linux MM, > > see: http://www.linux-mm.org/ . > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > The bug can be reliably reproduced in our platform with the > current upstream kernel(80aa76bcd364 Merge tag 'xfs-4.17-merge-4' of > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux). I'm happy to > help to test and debug. The error message is as following: Thanks for report! So what workload do you run to trigger this? Is it just a direct IO read to a buffer in a shared file mapping or something else? Honza > kernel BUG at /home/gavin/work-kernel/fs/ext4/inode.c:2126! > invalid opcode: 0000 [#1] SMP PTI > Modules linked in: veth ipt_MASQUERADE nf_nat_masquerade_ipv4 > nf_conntrack_netlink nfnetlink xfrm_user iptable_nat nf_conntrack_ipv4 > nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype xt_conntrack nf_nat > nf_conntrack br_netfilter bridge stp llc overlay xt_multiport > iptable_filter ip_tables x_tables cachefiles fscache esp6_offload esp6 > esp4_offload esp4 xfrm_algo nls_iso8859_1 intel_rapl sb_edac > x86_pkg_temp_thermal intel_powerclamp ipmi_ssif coretemp kvm_intel > nvidia_uvm(POE) mxm_wmi kvm joydev input_leds irqbypass intel_cstate > intel_rapl_perf mei_me ipmi_si shpchp lpc_ich mei acpi_power_meter > mac_hid wmi ib_iser rdma_cm iw_cm ib_cm ib_core iscsi_tcp libiscsi_tcp > libiscsi scsi_transport_iscsi ipmi_devintf sunrpc ipmi_msghandler > autofs4 btrfs zstd_compress raid10 raid456 async_raid6_recov > async_memcpy > async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 > multipath linear i2c_algo_bit nvidia_drm(POE) ses crct10dif_pclmul > crc32_pclmul nvidia_modeset(POE) ttm ghash_clmulni_intel enclosure > hid_generic uas pcbc scsi_transport_sas drm_kms_helper usbhid > aesni_intel hid aes_x86_64 usb_storage syscopyarea crypto_simd > sysfillrect mlx5_core cryptd nvidia(POE) sysimgblt glue_helper ixgbe > mlxfw megaraid_sas fb_sys_fops devlink dca ahci ptp drm libahci > pps_core mdio > CPU: 54 PID: 8938 Comm: kworker/u161:0 Tainted: P OE > 4.16.0-999-generic #201804102200 > > Workqueue: writeback wb_workfn (flush-8:0) > RIP: 0010:ext4_writepage+0x318/0x770 > RSP: 0018:ffffb514e76cb7f8 EFLAGS: 00010246 > RAX: 00500b4e8000026d RBX: 0000000000001000 RCX: ffff8c2cafba5000 > RDX: ffff8bec4069a020 RSI: ffffb514e76cbc28 RDI: ffffe91efcc8bd80 > RBP: ffffb514e76cb870 R08: 0000000000028115 R09: 00000000000280c0 > R10: 0000000000000002 R11: ffff8c2dbffd4000 R12: ffffe91efcc8bd80 > R13: ffff8bec40699ea8 R14: ffffb514e76cbc28 R15: ffffe91efcc8bd80 > FS: 0000000000000000(0000) GS:ffff8becbfc80000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007ffcc8f6f080 CR3: 0000004b9ce0a006 CR4: 00000000003606e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > ? rmap_walk+0x41/0x60 > ? page_mkclean+0x9f/0xb0 > ? invalid_page_referenced_vma+0x80/0x80 > __writepage+0x17/0x50 > write_cache_pages+0x228/0x4a0 > ? __wb_calc_thresh+0x140/0x140 > generic_writepages+0x61/0xa0 > ? _cond_resched+0x1a/0x50 > ? write_cache_pages+0x396/0x4a0 > ext4_writepages+0x1fc/0xe00 > ? ext4_writepages+0x1fc/0xe00 > ? generic_writepages+0x6d/0xa0 > ? fprop_fraction_percpu+0x2f/0x80 > do_writepages+0x1c/0x60 > ? do_writepages+0x1c/0x60 > __writeback_single_inode+0x45/0x320 > writeback_sb_inodes+0x266/0x580 > __writeback_inodes_wb+0x92/0xc0 > wb_writeback+0x282/0x310 > wb_workfn+0x1a3/0x440 > ? wb_workfn+0x1a3/0x440 > process_one_work+0x1db/0x3c0 > worker_thread+0x4b/0x420 > kthread+0x102/0x140 > ? rescuer_thread+0x380/0x380 > ? kthread_create_worker_on_cpu+0x70/0x70 > ret_from_fork+0x35/0x40 > Code: ff f6 c4 08 0f 85 58 ff ff ff e8 68 56 00 00 ba 00 10 00 00 31 > f6 41 bd fb ff ff ff e8 a2 9e ff ff 4c 89 e7 e8 fa 39 ea ff eb 8a <0f> > 0b c6 45 a8 00 e9 f0 fd ff ff 49 83 7c 24 10 00 0f 85 c7 03 > RIP: ext4_writepage+0x318/0x770 RSP: ffffb514e76cb7f8 > ---[ end trace 59d4e1a4b221404b ]--- -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-04-13 12:58 ` Jan Kara @ 2018-04-16 9:20 ` Gavin Guo 0 siblings, 0 replies; 16+ messages in thread From: Gavin Guo @ 2018-04-16 9:20 UTC (permalink / raw) To: Jan Kara; +Cc: Dan Williams, Linux MM, linux-fsdevel, linux-xfs, linux-ext4 On Fri, Apr 13, 2018 at 8:58 PM, Jan Kara <jack@suse.cz> wrote: > Hi! > > On Fri 13-04-18 20:39:06, Gavin Guo wrote: >> On Thu, Jan 4, 2018 at 7:33 PM, Jan Kara <jack@suse.cz> wrote: >> > >> > On Wed 03-01-18 20:56:32, Dan Williams wrote: >> > > On Wed, Jan 3, 2018 at 2:04 AM, Jan Kara <jack@suse.cz> wrote: >> > > > Hello, >> > > > >> > > > Over the years I have seen so far unexplained crashed in filesystem's >> > > > (ext4, xfs) writeback path due to dirty pages without buffers attached to >> > > > them (see [1] and [2] for relatively recent reports). This was confusing as >> > > > reclaim takes care not to strip buffers from a dirty page and both >> > > > filesystems do add buffers to a page when it is first written to - in >> > > > ->page_mkwrite() and ->write_begin callbacks. >> > > > >> > > > Recently I have come across a code path that is probably leading to this >> > > > inconsistent state and I'd like to discuss how to best fix the problem >> > > > because it's not obvious to me. Consider the following race: >> > > > >> > > > CPU1 CPU2 >> > > > >> > > > addr = mmap(file1, MAP_SHARED, ...); >> > > > fd2 = open(file2, O_DIRECT | O_RDONLY); >> > > > read(fd2, addr, len) >> > > > do_direct_IO() >> > > > page = dio_get_page() >> > > > dio_refill_pages() >> > > > iov_iter_get_pages() >> > > > get_user_pages_fast() >> > > > - page fault >> > > > ->page_mkwrite() >> > > > block_page_mkwrite() >> > > > lock_page(page); >> > > > - attaches buffers to page >> > > > - makes sure blocks are allocated >> > > > set_page_dirty(page) >> > > > - install writeable PTE >> > > > unlock_page(page); >> > > > submit_page_section(page) >> > > > - submits bio with 'page' as a buffer >> > > > kswapd reclaims pages: >> > > > ... >> > > > shrink_page_list() >> > > > trylock_page(page) - this is the >> > > > page CPU1 has just faulted in >> > > > try_to_unmap(page) >> > > > pageout(page); >> > > > clear_page_dirty_for_io(page); >> > > > ->writepage() >> > > > - let's assume page got written >> > > > out fast enough, alternatively >> > > > we could get to the same path as >> > > > soon as the page IO completes >> > > > if (page_has_private(page)) { >> > > > try_to_release_page(page) >> > > > - reclaims buffers from the >> > > > page >> > > > __remove_mapping(page) >> > > > - fails as DIO code still >> > > > holds page reference >> > > > ... >> > > > >> > > > eventually read completes >> > > > dio_bio_complete(bio) >> > > > set_page_dirty_lock(page) >> > > > Bummer, we've just marked the page as dirty without having buffers. >> > > > Eventually writeback will find it and filesystem will complain... >> > > > >> > > > Am I missing something? >> > > > >> > > > The problem here is that filesystems fundamentally assume that a page can >> > > > be written to only between ->write_begin - ->write_end (in this interval >> > > > the page is locked), or between ->page_mkwrite - ->writepage and above is >> > > > an example where this does not hold because when a page reference is >> > > > acquired through get_user_pages(), page can get written to by the holder of >> > > > the reference and dirtied even after it has been unmapped from page tables >> > > > and ->writepage has been called. This is not only a cosmetic issue leading >> > > > to assertion failure but it can also lead to data loss, data corruption, or >> > > > other unpleasant surprises as filesystems assume page contents cannot be >> > > > modified until either ->write_begin() or ->page_mkwrite gets called and >> > > > those calls are serialized by proper locking with problematic operations >> > > > such as hole punching etc. >> > > > >> > > > I'm not sure how to fix this problem. We could 'simulate' a writeable page >> > > > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a >> > > > virtual address of the fault, don't hold mmap_sem, etc., possibly >> > > > expensive, but it would make filesystems happy. Data stored by GUP user >> > > > (e.g. read by DIO in the above case) could still get lost if someone e.g. >> > > > punched hole under the buffer or otherwise messed with the underlying >> > > > storage of the page while DIO was running but arguably users could expect >> > > > such outcome. >> > > > >> > > > Another possible solution would be to make sure page is writeably mapped >> > > > until GUP user drops its reference. That would be arguably cleaner but >> > > > probably that would mean we have to track number of writeable GUP page >> > > > references separately (no space space in struct page is a problem here) and >> > > > block page_mkclean() until they are dropped. Also for long term GUP users >> > > > like Infiniband or V4L we'd have to come up with some solution as we should >> > > > not block page_mkclean() for so long. >> > > >> > > Do we need to block page_mkclean, or could we defer buffer reclaiming >> > > to the last put of the page? >> > >> > As I wrote to Dave the problem is no so much with reclaiming of buffers but >> > with the fact filesystems don't expect page can be dirtied after >> > page_mkclean() is finished. >> > >> > > I think once we have the "register memory with lease" mechanism for >> > > Infiniband we could expand it to the page cache case. The problem is >> > > the regression this would cause with userspace that expects it can >> > > maintain file backed memory registrations indefinitely. >> > > >> > > What are the implications of holding off page_mkclean or release >> > > buffers indefinitely? >> > >> > Bad. You cannot write the page to disk until page_mkclean() finishes as >> > page_mkclean() is part of clear_page_dirty_for_io(). And we really do need >> > that functionality there e.g. to make sure tail of the last page in the >> > file is properly zeroed out, storage with DIF/DIX can compute checksum of >> > the data safely before submitting it to the device etc. >> > >> > > Is an indefinite / interruptible sleep waiting for the 'put' event of >> > > a get_user_pages() page unacceptable? The current case that the file >> > > contents will not be coherent with respect to in-flight RDMA, perhaps >> > > waiting for that to complete is better than cleaning buffers from the >> > > page prematurely. >> > >> > Yeah, indefinite sleep is really a no-go. >> > >> > > > As a side note DAX needs some solution for GUP users as well. The problems >> > > > are similar there in nature, just much easier to hit. So at least a >> > > > solution for long-term GUP users can (and I strongly believe should) be >> > > > shared between standard and DAX paths. >> > > >> > > In the DAX case we rely on the fact that when the page goes idle we >> > > only need to worry about the filesytem block map changing, the page >> > > won't get reallocated somewhere else. We can't use page idle as an >> > > event in this case, however, if the page reference count is one then >> > > the DIO code can know that it has the page exclusively, so maybe DAX >> > > and non-DAX can share the page count == 1 event notification. >> > >> > The races I describe do not need to involve truncate / hole punching. It is >> > just enough to race with page writeback. So page references are of no use >> > here. We would have to specifically track number of references acquired by >> > GUP or something like that. So what I wanted to share with DAX is the >> > long-term pin handling, the rest is unclear for now. >> > >> > Honza >> > -- >> > Jan Kara <jack@suse.com> >> > SUSE Labs, CR >> > >> > -- >> > To unsubscribe, send a message with 'unsubscribe linux-mm' in >> > the body to majordomo@kvack.org. For more info on Linux MM, >> > see: http://www.linux-mm.org/ . >> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> >> >> The bug can be reliably reproduced in our platform with the >> current upstream kernel(80aa76bcd364 Merge tag 'xfs-4.17-merge-4' of >> git://git.kernel.org/pub/scm/fs/xfs/xfs-linux). I'm happy to >> help to test and debug. The error message is as following: > > Thanks for report! So what workload do you run to trigger this? Is it just > a direct IO read to a buffer in a shared file mapping or something else? > > Honza > >> kernel BUG at /home/gavin/work-kernel/fs/ext4/inode.c:2126! >> invalid opcode: 0000 [#1] SMP PTI >> Modules linked in: veth ipt_MASQUERADE nf_nat_masquerade_ipv4 >> nf_conntrack_netlink nfnetlink xfrm_user iptable_nat nf_conntrack_ipv4 >> nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype xt_conntrack nf_nat >> nf_conntrack br_netfilter bridge stp llc overlay xt_multiport >> iptable_filter ip_tables x_tables cachefiles fscache esp6_offload esp6 >> esp4_offload esp4 xfrm_algo nls_iso8859_1 intel_rapl sb_edac >> x86_pkg_temp_thermal intel_powerclamp ipmi_ssif coretemp kvm_intel >> nvidia_uvm(POE) mxm_wmi kvm joydev input_leds irqbypass intel_cstate >> intel_rapl_perf mei_me ipmi_si shpchp lpc_ich mei acpi_power_meter >> mac_hid wmi ib_iser rdma_cm iw_cm ib_cm ib_core iscsi_tcp libiscsi_tcp >> libiscsi scsi_transport_iscsi ipmi_devintf sunrpc ipmi_msghandler >> autofs4 btrfs zstd_compress raid10 raid456 async_raid6_recov >> async_memcpy >> async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 >> multipath linear i2c_algo_bit nvidia_drm(POE) ses crct10dif_pclmul >> crc32_pclmul nvidia_modeset(POE) ttm ghash_clmulni_intel enclosure >> hid_generic uas pcbc scsi_transport_sas drm_kms_helper usbhid >> aesni_intel hid aes_x86_64 usb_storage syscopyarea crypto_simd >> sysfillrect mlx5_core cryptd nvidia(POE) sysimgblt glue_helper ixgbe >> mlxfw megaraid_sas fb_sys_fops devlink dca ahci ptp drm libahci >> pps_core mdio >> CPU: 54 PID: 8938 Comm: kworker/u161:0 Tainted: P OE >> 4.16.0-999-generic #201804102200 >> >> Workqueue: writeback wb_workfn (flush-8:0) >> RIP: 0010:ext4_writepage+0x318/0x770 >> RSP: 0018:ffffb514e76cb7f8 EFLAGS: 00010246 >> RAX: 00500b4e8000026d RBX: 0000000000001000 RCX: ffff8c2cafba5000 >> RDX: ffff8bec4069a020 RSI: ffffb514e76cbc28 RDI: ffffe91efcc8bd80 >> RBP: ffffb514e76cb870 R08: 0000000000028115 R09: 00000000000280c0 >> R10: 0000000000000002 R11: ffff8c2dbffd4000 R12: ffffe91efcc8bd80 >> R13: ffff8bec40699ea8 R14: ffffb514e76cbc28 R15: ffffe91efcc8bd80 >> FS: 0000000000000000(0000) GS:ffff8becbfc80000(0000) >> knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007ffcc8f6f080 CR3: 0000004b9ce0a006 CR4: 00000000003606e0 >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >> Call Trace: >> ? rmap_walk+0x41/0x60 >> ? page_mkclean+0x9f/0xb0 >> ? invalid_page_referenced_vma+0x80/0x80 >> __writepage+0x17/0x50 >> write_cache_pages+0x228/0x4a0 >> ? __wb_calc_thresh+0x140/0x140 >> generic_writepages+0x61/0xa0 >> ? _cond_resched+0x1a/0x50 >> ? write_cache_pages+0x396/0x4a0 >> ext4_writepages+0x1fc/0xe00 >> ? ext4_writepages+0x1fc/0xe00 >> ? generic_writepages+0x6d/0xa0 >> ? fprop_fraction_percpu+0x2f/0x80 >> do_writepages+0x1c/0x60 >> ? do_writepages+0x1c/0x60 >> __writeback_single_inode+0x45/0x320 >> writeback_sb_inodes+0x266/0x580 >> __writeback_inodes_wb+0x92/0xc0 >> wb_writeback+0x282/0x310 >> wb_workfn+0x1a3/0x440 >> ? wb_workfn+0x1a3/0x440 >> process_one_work+0x1db/0x3c0 >> worker_thread+0x4b/0x420 >> kthread+0x102/0x140 >> ? rescuer_thread+0x380/0x380 >> ? kthread_create_worker_on_cpu+0x70/0x70 >> ret_from_fork+0x35/0x40 >> Code: ff f6 c4 08 0f 85 58 ff ff ff e8 68 56 00 00 ba 00 10 00 00 31 >> f6 41 bd fb ff ff ff e8 a2 9e ff ff 4c 89 e7 e8 fa 39 ea ff eb 8a <0f> >> 0b c6 45 a8 00 e9 f0 fd ff ff 49 83 7c 24 10 00 0f 85 c7 03 >> RIP: ext4_writepage+0x318/0x770 RSP: ffffb514e76cb7f8 >> ---[ end trace 59d4e1a4b221404b ]--- > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR ############################## ## The reproducing scenario ## ############################## The reproducer is creating eight processes by the mpirun and each process is bound to the underlying GPU computation unit by the cudaSetDevice() interface. The scenario is that each process creates a 8K bytes buffer by the cudaMalloc() and sends/receives the 8K bytes buffer in a circular buffer behavior by the MPI_Isend()/MPI_Irecv() like: P0 -> P1 -> P2 -> P3 -> P4 -> P5 -> P6 -> P7-> to P0 The sending/receiving buffer is iterated 1000 times, then to proceed to clean up the resource allocated to the cuda device by: cudaDeviceReset(); MPI_Finalize(); However, the sequence is actually controversial as the cudaDeviceReset() already cleaned up the resource, including the 8K bytes memory allocated by the cudaMalloc(), MPI_Finalize() could access the memory allocated by cudaMalloc(). Finally, the MPI_Finalize() accidentally triggers the kernel bug. I tried to look up the implementation of the MPI library, unfortunately, the exact mechanism corresponded to the try_to_free_buffers()/set_page_dirty() is still unknown. ############################## ##### The kernel testing ##### ############################## 1). I tried the patch which tries to capture the dirty page scenario when the page is released, however, there is no warning message when the bug is successfully reproduced. So, it eliminates the possibility that the ext4_releasepage() removes the buffer of a dirty page. diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 3d4d1dccc8a1..9310277c8e3e 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3384,6 +3384,11 @@ static int ext4_releasepage(struct page *page, gfp_t wait) trace_ext4_releasepage(page); + if (PageDirty(page)) { + WARN_ON(1); + return 0; + } + /* Page has dirty journalled data -> cannot release */ if (PageChecked(page)) return 0; diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 930aa0d19761..3bd1a605853f 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1134,6 +1134,11 @@ static int bdev_try_to_free_page(struct super_block *sb, struct page *page, { journal_t *journal = EXT4_SB(sb)->s_journal; + if (PageDirty(page)) { + WARN_ON(1); + return 0; + } + WARN_ON(PageChecked(page)); if (!page_has_buffers(page)) return 0; 2). Then, I proceeded to try the patch mentioned in link[1] and found the patch was already merged into the kernel v4.10-rc1 with commit id: 6dcc693bc57f ext4: warn when page is dirtied without buffers According to the code: static int ext4_set_page_dirty(struct page *page) { WARN_ON_ONCE(!PageLocked(page) && !PageDirty(page)); WARN_ON_ONCE(!page_has_buffers(page)); return __set_page_dirty_buffers(page); } I also cannot find the warning message[2] with the "WARN_ON_ONCE(!page_has_buffers(page))" when reproduced with the latest upstream kernel. With the patch in the current kernel, I think the first scenario, CPU1 is doing direct IO and CPU2 is doing the reclaim, also can be captured in the ext4_set_page_dirty(). And there is no error message related to the warning, so, it seems not related to this case. Am I missing anything? > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) set_page_dirty int (*spd)(struct page *) = mapping->a_ops->set_page_dirty; return (*spd)(page); ext4_set_page_dirty WARN_ON_ONCE(!page_has_buffers(page)); > Bummer, we've just marked the page as dirty without having buffers. > Eventually writeback will find it and filesystem will complain... There could be other path triggering the bug. Any idea where to add the debug message? ############################## ######### Reference ########## ############################## [1]. kernel BUG at fs/ext4/inode.c:2428! https://patchwork.ozlabs.org/patch/697715/ [2]. coredump message of latest upstream kernel http://paste.ubuntu.com/p/FfDk9cChdX/ ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-03 10:04 ` Jan Kara @ 2018-01-04 5:59 ` Dave Chinner -1 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2018-01-04 5:59 UTC (permalink / raw) To: Jan Kara; +Cc: linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > Hello, > > Over the years I have seen so far unexplained crashed in filesystem's > (ext4, xfs) writeback path due to dirty pages without buffers attached to > them (see [1] and [2] for relatively recent reports). This was confusing as > reclaim takes care not to strip buffers from a dirty page and both > filesystems do add buffers to a page when it is first written to - in > ->page_mkwrite() and ->write_begin callbacks. > > Recently I have come across a code path that is probably leading to this > inconsistent state and I'd like to discuss how to best fix the problem > because it's not obvious to me. Consider the following race: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > page = dio_get_page() > dio_refill_pages() > iov_iter_get_pages() > get_user_pages_fast() > - page fault > ->page_mkwrite() > block_page_mkwrite() > lock_page(page); > - attaches buffers to page > - makes sure blocks are allocated > set_page_dirty(page) > - install writeable PTE > unlock_page(page); > submit_page_section(page) > - submits bio with 'page' as a buffer > kswapd reclaims pages: > ... > shrink_page_list() > trylock_page(page) - this is the > page CPU1 has just faulted in > try_to_unmap(page) > pageout(page); > clear_page_dirty_for_io(page); > ->writepage() > - let's assume page got written > out fast enough, alternatively > we could get to the same path as > soon as the page IO completes > if (page_has_private(page)) { > try_to_release_page(page) > - reclaims buffers from the > page > __remove_mapping(page) > - fails as DIO code still > holds page reference > ... > > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > Bummer, we've just marked the page as dirty without having buffers. > Eventually writeback will find it and filesystem will complain... > > Am I missing something? My first question is why is kswapd trying to reclaim a page with an elevated active reference count? i.e. there are active references the VM *doesn't own* to the page, which means that there may well a user that expects the state on the page (e.g. the page private data that the active reference instantiated!) to remain intact until it drops it's active reference. That seems like really basic reference counting/reclaim bug to me: we shouldn't ever attempt to reclaim and free an object while there are active external references to it that object, regardless of the subsystem the object belongs to.... Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers @ 2018-01-04 5:59 ` Dave Chinner 0 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2018-01-04 5:59 UTC (permalink / raw) To: Jan Kara; +Cc: linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > Hello, > > Over the years I have seen so far unexplained crashed in filesystem's > (ext4, xfs) writeback path due to dirty pages without buffers attached to > them (see [1] and [2] for relatively recent reports). This was confusing as > reclaim takes care not to strip buffers from a dirty page and both > filesystems do add buffers to a page when it is first written to - in > ->page_mkwrite() and ->write_begin callbacks. > > Recently I have come across a code path that is probably leading to this > inconsistent state and I'd like to discuss how to best fix the problem > because it's not obvious to me. Consider the following race: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > page = dio_get_page() > dio_refill_pages() > iov_iter_get_pages() > get_user_pages_fast() > - page fault > ->page_mkwrite() > block_page_mkwrite() > lock_page(page); > - attaches buffers to page > - makes sure blocks are allocated > set_page_dirty(page) > - install writeable PTE > unlock_page(page); > submit_page_section(page) > - submits bio with 'page' as a buffer > kswapd reclaims pages: > ... > shrink_page_list() > trylock_page(page) - this is the > page CPU1 has just faulted in > try_to_unmap(page) > pageout(page); > clear_page_dirty_for_io(page); > ->writepage() > - let's assume page got written > out fast enough, alternatively > we could get to the same path as > soon as the page IO completes > if (page_has_private(page)) { > try_to_release_page(page) > - reclaims buffers from the > page > __remove_mapping(page) > - fails as DIO code still > holds page reference > ... > > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > Bummer, we've just marked the page as dirty without having buffers. > Eventually writeback will find it and filesystem will complain... > > Am I missing something? My first question is why is kswapd trying to reclaim a page with an elevated active reference count? i.e. there are active references the VM *doesn't own* to the page, which means that there may well a user that expects the state on the page (e.g. the page private data that the active reference instantiated!) to remain intact until it drops it's active reference. That seems like really basic reference counting/reclaim bug to me: we shouldn't ever attempt to reclaim and free an object while there are active external references to it that object, regardless of the subsystem the object belongs to.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-04 5:59 ` Dave Chinner @ 2018-01-04 8:52 ` Jan Kara -1 siblings, 0 replies; 16+ messages in thread From: Jan Kara @ 2018-01-04 8:52 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams On Thu 04-01-18 16:59:19, Dave Chinner wrote: > On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > > Hello, > > > > Over the years I have seen so far unexplained crashed in filesystem's > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > them (see [1] and [2] for relatively recent reports). This was confusing as > > reclaim takes care not to strip buffers from a dirty page and both > > filesystems do add buffers to a page when it is first written to - in > > ->page_mkwrite() and ->write_begin callbacks. > > > > Recently I have come across a code path that is probably leading to this > > inconsistent state and I'd like to discuss how to best fix the problem > > because it's not obvious to me. Consider the following race: > > > > CPU1 CPU2 > > > > addr = mmap(file1, MAP_SHARED, ...); > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > read(fd2, addr, len) > > do_direct_IO() > > page = dio_get_page() > > dio_refill_pages() > > iov_iter_get_pages() > > get_user_pages_fast() > > - page fault > > ->page_mkwrite() > > block_page_mkwrite() > > lock_page(page); > > - attaches buffers to page > > - makes sure blocks are allocated > > set_page_dirty(page) > > - install writeable PTE > > unlock_page(page); > > submit_page_section(page) > > - submits bio with 'page' as a buffer > > kswapd reclaims pages: > > ... > > shrink_page_list() > > trylock_page(page) - this is the > > page CPU1 has just faulted in > > try_to_unmap(page) > > pageout(page); > > clear_page_dirty_for_io(page); > > ->writepage() > > - let's assume page got written > > out fast enough, alternatively > > we could get to the same path as > > soon as the page IO completes > > if (page_has_private(page)) { > > try_to_release_page(page) > > - reclaims buffers from the > > page > > __remove_mapping(page) > > - fails as DIO code still > > holds page reference > > ... > > > > eventually read completes > > dio_bio_complete(bio) > > set_page_dirty_lock(page) > > Bummer, we've just marked the page as dirty without having buffers. > > Eventually writeback will find it and filesystem will complain... > > > > Am I missing something? > > My first question is why is kswapd trying to reclaim a page with an > elevated active reference count? i.e. there are active references > the VM *doesn't own* to the page, which means that there may well > a user that expects the state on the page (e.g. the page private > data that the active reference instantiated!) to remain intact until > it drops it's active reference. Page private data (and most of page state) is protected by a page lock, not by a page reference. So reclaim (which is holding the page lock) is free to try to reclaim page private data by calling ->releasepage callback. That being said you are right that the attempt to reclaim a page with active references is futile. But the problem is that we don't know how many page references are actually left before we unmap the page from page tables (each page table entry holds a page reference) and free page private data (as that may hold page reference as well - e.g. attach_page_buffers() acquires page reference). So checking page references in advance is difficult. Furthermore the core of the problem is not in the fact that page buffers are reclaimed. That just makes it visible. The real problem is that page can be written to by a GUP user while it is neither writeably mapped in page tables nor prepared with ->write_begin. So a similar race violating filesystem's assumptions can be like: CPU1 CPU2 addr = mmap(file1, MAP_SHARED, ...); fd2 = open(file2, O_DIRECT | O_RDONLY); read(fd2, addr, len) do_direct_IO() ... page = get_user_pages_fast() - page fault handled submit_page_section(page) - submits bio with 'page' as a buffer ordinary writeback: writepages(file1) clear_page_dirty_for_io(page) - page gets writeprotected in page tables ... eventually read completes dio_bio_complete(bio) set_page_dirty_lock(page) And a race like this is enough to cause data corruption if we are unlucky. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers @ 2018-01-04 8:52 ` Jan Kara 0 siblings, 0 replies; 16+ messages in thread From: Jan Kara @ 2018-01-04 8:52 UTC (permalink / raw) To: Dave Chinner Cc: Jan Kara, linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams On Thu 04-01-18 16:59:19, Dave Chinner wrote: > On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > > Hello, > > > > Over the years I have seen so far unexplained crashed in filesystem's > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > them (see [1] and [2] for relatively recent reports). This was confusing as > > reclaim takes care not to strip buffers from a dirty page and both > > filesystems do add buffers to a page when it is first written to - in > > ->page_mkwrite() and ->write_begin callbacks. > > > > Recently I have come across a code path that is probably leading to this > > inconsistent state and I'd like to discuss how to best fix the problem > > because it's not obvious to me. Consider the following race: > > > > CPU1 CPU2 > > > > addr = mmap(file1, MAP_SHARED, ...); > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > read(fd2, addr, len) > > do_direct_IO() > > page = dio_get_page() > > dio_refill_pages() > > iov_iter_get_pages() > > get_user_pages_fast() > > - page fault > > ->page_mkwrite() > > block_page_mkwrite() > > lock_page(page); > > - attaches buffers to page > > - makes sure blocks are allocated > > set_page_dirty(page) > > - install writeable PTE > > unlock_page(page); > > submit_page_section(page) > > - submits bio with 'page' as a buffer > > kswapd reclaims pages: > > ... > > shrink_page_list() > > trylock_page(page) - this is the > > page CPU1 has just faulted in > > try_to_unmap(page) > > pageout(page); > > clear_page_dirty_for_io(page); > > ->writepage() > > - let's assume page got written > > out fast enough, alternatively > > we could get to the same path as > > soon as the page IO completes > > if (page_has_private(page)) { > > try_to_release_page(page) > > - reclaims buffers from the > > page > > __remove_mapping(page) > > - fails as DIO code still > > holds page reference > > ... > > > > eventually read completes > > dio_bio_complete(bio) > > set_page_dirty_lock(page) > > Bummer, we've just marked the page as dirty without having buffers. > > Eventually writeback will find it and filesystem will complain... > > > > Am I missing something? > > My first question is why is kswapd trying to reclaim a page with an > elevated active reference count? i.e. there are active references > the VM *doesn't own* to the page, which means that there may well > a user that expects the state on the page (e.g. the page private > data that the active reference instantiated!) to remain intact until > it drops it's active reference. Page private data (and most of page state) is protected by a page lock, not by a page reference. So reclaim (which is holding the page lock) is free to try to reclaim page private data by calling ->releasepage callback. That being said you are right that the attempt to reclaim a page with active references is futile. But the problem is that we don't know how many page references are actually left before we unmap the page from page tables (each page table entry holds a page reference) and free page private data (as that may hold page reference as well - e.g. attach_page_buffers() acquires page reference). So checking page references in advance is difficult. Furthermore the core of the problem is not in the fact that page buffers are reclaimed. That just makes it visible. The real problem is that page can be written to by a GUP user while it is neither writeably mapped in page tables nor prepared with ->write_begin. So a similar race violating filesystem's assumptions can be like: CPU1 CPU2 addr = mmap(file1, MAP_SHARED, ...); fd2 = open(file2, O_DIRECT | O_RDONLY); read(fd2, addr, len) do_direct_IO() ... page = get_user_pages_fast() - page fault handled submit_page_section(page) - submits bio with 'page' as a buffer ordinary writeback: writepages(file1) clear_page_dirty_for_io(page) - page gets writeprotected in page tables ... eventually read completes dio_bio_complete(bio) set_page_dirty_lock(page) And a race like this is enough to cause data corruption if we are unlucky. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-04 8:52 ` Jan Kara @ 2018-01-04 10:08 ` Dave Chinner -1 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2018-01-04 10:08 UTC (permalink / raw) To: Jan Kara; +Cc: linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams On Thu, Jan 04, 2018 at 09:52:44AM +0100, Jan Kara wrote: > On Thu 04-01-18 16:59:19, Dave Chinner wrote: > > On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > > > Hello, > > > > > > Over the years I have seen so far unexplained crashed in filesystem's > > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > > them (see [1] and [2] for relatively recent reports). This was confusing as > > > reclaim takes care not to strip buffers from a dirty page and both > > > filesystems do add buffers to a page when it is first written to - in > > > ->page_mkwrite() and ->write_begin callbacks. > > > > > > Recently I have come across a code path that is probably leading to this > > > inconsistent state and I'd like to discuss how to best fix the problem > > > because it's not obvious to me. Consider the following race: > > > > > > CPU1 CPU2 > > > > > > addr = mmap(file1, MAP_SHARED, ...); > > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > > read(fd2, addr, len) > > > do_direct_IO() > > > page = dio_get_page() > > > dio_refill_pages() > > > iov_iter_get_pages() > > > get_user_pages_fast() > > > - page fault > > > ->page_mkwrite() > > > block_page_mkwrite() > > > lock_page(page); > > > - attaches buffers to page > > > - makes sure blocks are allocated > > > set_page_dirty(page) > > > - install writeable PTE > > > unlock_page(page); > > > submit_page_section(page) > > > - submits bio with 'page' as a buffer > > > kswapd reclaims pages: > > > ... > > > shrink_page_list() > > > trylock_page(page) - this is the > > > page CPU1 has just faulted in > > > try_to_unmap(page) > > > pageout(page); > > > clear_page_dirty_for_io(page); > > > ->writepage() > > > - let's assume page got written > > > out fast enough, alternatively > > > we could get to the same path as > > > soon as the page IO completes > > > if (page_has_private(page)) { > > > try_to_release_page(page) > > > - reclaims buffers from the > > > page > > > __remove_mapping(page) > > > - fails as DIO code still > > > holds page reference > > > ... > > > > > > eventually read completes > > > dio_bio_complete(bio) > > > set_page_dirty_lock(page) > > > Bummer, we've just marked the page as dirty without having buffers. > > > Eventually writeback will find it and filesystem will complain... > > > > > > Am I missing something? > > > > My first question is why is kswapd trying to reclaim a page with an > > elevated active reference count? i.e. there are active references > > the VM *doesn't own* to the page, which means that there may well > > a user that expects the state on the page (e.g. the page private > > data that the active reference instantiated!) to remain intact until > > it drops it's active reference. > > Page private data (and most of page state) is protected by a page lock, not > by a page reference. So reclaim (which is holding the page lock) is free to > try to reclaim page private data by calling ->releasepage callback. Page private data is "owned" by whoever put the private data there. Manipulating the fields and state that says there is private data on the page is protected by the page lock. > That being said you are right that the attempt to reclaim a page with > active references is futile. But the problem is that we don't know how many > page references are actually left before we unmap the page from page tables > (each page table entry holds a page reference) and free page private data > (as that may hold page reference as well - e.g. attach_page_buffers() > acquires page reference). So checking page references in advance is > difficult. perhaps we need separate accounting of internal and active references (kinda like superblocks), where active references prevent reclaim because they require the current state to be maintained until the reference is dropped, whilst internal references simply prevent the page from being freed until they are released. > Furthermore the core of the problem is not in the fact that page buffers > are reclaimed. That just makes it visible. The real problem is that page can > be written to by a GUP user while it is neither writeably mapped in page > tables nor prepared with ->write_begin. So a similar race violating > filesystem's assumptions can be like: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > ... > page = get_user_pages_fast() > - page fault handled > submit_page_section(page) > - submits bio with 'page' as a buffer > ordinary writeback: > writepages(file1) > clear_page_dirty_for_io(page) > - page gets writeprotected in > page tables > ... > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > > And a race like this is enough to cause data corruption if we are unlucky. Hmmm. if that's the case then we probably need a page flag to indicate the page cannot be cleaned, unmapped or reclaimed by anything until the GUP reference owner clears that flag. It seems analagous to the PageWriteback flag and teh way we avoid certain things when we know the page is under IO (e.g. immediate reclaim :P). Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers @ 2018-01-04 10:08 ` Dave Chinner 0 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2018-01-04 10:08 UTC (permalink / raw) To: Jan Kara; +Cc: linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams On Thu, Jan 04, 2018 at 09:52:44AM +0100, Jan Kara wrote: > On Thu 04-01-18 16:59:19, Dave Chinner wrote: > > On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > > > Hello, > > > > > > Over the years I have seen so far unexplained crashed in filesystem's > > > (ext4, xfs) writeback path due to dirty pages without buffers attached to > > > them (see [1] and [2] for relatively recent reports). This was confusing as > > > reclaim takes care not to strip buffers from a dirty page and both > > > filesystems do add buffers to a page when it is first written to - in > > > ->page_mkwrite() and ->write_begin callbacks. > > > > > > Recently I have come across a code path that is probably leading to this > > > inconsistent state and I'd like to discuss how to best fix the problem > > > because it's not obvious to me. Consider the following race: > > > > > > CPU1 CPU2 > > > > > > addr = mmap(file1, MAP_SHARED, ...); > > > fd2 = open(file2, O_DIRECT | O_RDONLY); > > > read(fd2, addr, len) > > > do_direct_IO() > > > page = dio_get_page() > > > dio_refill_pages() > > > iov_iter_get_pages() > > > get_user_pages_fast() > > > - page fault > > > ->page_mkwrite() > > > block_page_mkwrite() > > > lock_page(page); > > > - attaches buffers to page > > > - makes sure blocks are allocated > > > set_page_dirty(page) > > > - install writeable PTE > > > unlock_page(page); > > > submit_page_section(page) > > > - submits bio with 'page' as a buffer > > > kswapd reclaims pages: > > > ... > > > shrink_page_list() > > > trylock_page(page) - this is the > > > page CPU1 has just faulted in > > > try_to_unmap(page) > > > pageout(page); > > > clear_page_dirty_for_io(page); > > > ->writepage() > > > - let's assume page got written > > > out fast enough, alternatively > > > we could get to the same path as > > > soon as the page IO completes > > > if (page_has_private(page)) { > > > try_to_release_page(page) > > > - reclaims buffers from the > > > page > > > __remove_mapping(page) > > > - fails as DIO code still > > > holds page reference > > > ... > > > > > > eventually read completes > > > dio_bio_complete(bio) > > > set_page_dirty_lock(page) > > > Bummer, we've just marked the page as dirty without having buffers. > > > Eventually writeback will find it and filesystem will complain... > > > > > > Am I missing something? > > > > My first question is why is kswapd trying to reclaim a page with an > > elevated active reference count? i.e. there are active references > > the VM *doesn't own* to the page, which means that there may well > > a user that expects the state on the page (e.g. the page private > > data that the active reference instantiated!) to remain intact until > > it drops it's active reference. > > Page private data (and most of page state) is protected by a page lock, not > by a page reference. So reclaim (which is holding the page lock) is free to > try to reclaim page private data by calling ->releasepage callback. Page private data is "owned" by whoever put the private data there. Manipulating the fields and state that says there is private data on the page is protected by the page lock. > That being said you are right that the attempt to reclaim a page with > active references is futile. But the problem is that we don't know how many > page references are actually left before we unmap the page from page tables > (each page table entry holds a page reference) and free page private data > (as that may hold page reference as well - e.g. attach_page_buffers() > acquires page reference). So checking page references in advance is > difficult. perhaps we need separate accounting of internal and active references (kinda like superblocks), where active references prevent reclaim because they require the current state to be maintained until the reference is dropped, whilst internal references simply prevent the page from being freed until they are released. > Furthermore the core of the problem is not in the fact that page buffers > are reclaimed. That just makes it visible. The real problem is that page can > be written to by a GUP user while it is neither writeably mapped in page > tables nor prepared with ->write_begin. So a similar race violating > filesystem's assumptions can be like: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > ... > page = get_user_pages_fast() > - page fault handled > submit_page_section(page) > - submits bio with 'page' as a buffer > ordinary writeback: > writepages(file1) > clear_page_dirty_for_io(page) > - page gets writeprotected in > page tables > ... > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > > And a race like this is enough to cause data corruption if we are unlucky. Hmmm. if that's the case then we probably need a page flag to indicate the page cannot be cleaned, unmapped or reclaimed by anything until the GUP reference owner clears that flag. It seems analagous to the PageWriteback flag and teh way we avoid certain things when we know the page is under IO (e.g. immediate reclaim :P). Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Filesystem crashes due to pages without buffers 2018-01-03 10:04 ` Jan Kara ` (2 preceding siblings ...) (?) @ 2018-01-04 6:10 ` Leon Romanovsky -1 siblings, 0 replies; 16+ messages in thread From: Leon Romanovsky @ 2018-01-04 6:10 UTC (permalink / raw) To: Jan Kara Cc: linux-mm, linux-fsdevel, linux-xfs, linux-ext4, Dan Williams, RDMA mailing list, Majd Dibbiny [-- Attachment #1: Type: text/plain, Size: 9537 bytes --] On Wed, Jan 03, 2018 at 11:04:30AM +0100, Jan Kara wrote: > Hello, > > Over the years I have seen so far unexplained crashed in filesystem's > (ext4, xfs) writeback path due to dirty pages without buffers attached to > them (see [1] and [2] for relatively recent reports). This was confusing as > reclaim takes care not to strip buffers from a dirty page and both > filesystems do add buffers to a page when it is first written to - in > ->page_mkwrite() and ->write_begin callbacks. > > Recently I have come across a code path that is probably leading to this > inconsistent state and I'd like to discuss how to best fix the problem > because it's not obvious to me. Consider the following race: > > CPU1 CPU2 > > addr = mmap(file1, MAP_SHARED, ...); > fd2 = open(file2, O_DIRECT | O_RDONLY); > read(fd2, addr, len) > do_direct_IO() > page = dio_get_page() > dio_refill_pages() > iov_iter_get_pages() > get_user_pages_fast() > - page fault > ->page_mkwrite() > block_page_mkwrite() > lock_page(page); > - attaches buffers to page > - makes sure blocks are allocated > set_page_dirty(page) > - install writeable PTE > unlock_page(page); > submit_page_section(page) > - submits bio with 'page' as a buffer > kswapd reclaims pages: > ... > shrink_page_list() > trylock_page(page) - this is the > page CPU1 has just faulted in > try_to_unmap(page) > pageout(page); > clear_page_dirty_for_io(page); > ->writepage() > - let's assume page got written > out fast enough, alternatively > we could get to the same path as > soon as the page IO completes > if (page_has_private(page)) { > try_to_release_page(page) > - reclaims buffers from the > page > __remove_mapping(page) > - fails as DIO code still > holds page reference > ... > > eventually read completes > dio_bio_complete(bio) > set_page_dirty_lock(page) > Bummer, we've just marked the page as dirty without having buffers. > Eventually writeback will find it and filesystem will complain... > > Am I missing something? > > The problem here is that filesystems fundamentally assume that a page can > be written to only between ->write_begin - ->write_end (in this interval > the page is locked), or between ->page_mkwrite - ->writepage and above is > an example where this does not hold because when a page reference is > acquired through get_user_pages(), page can get written to by the holder of > the reference and dirtied even after it has been unmapped from page tables > and ->writepage has been called. This is not only a cosmetic issue leading > to assertion failure but it can also lead to data loss, data corruption, or > other unpleasant surprises as filesystems assume page contents cannot be > modified until either ->write_begin() or ->page_mkwrite gets called and > those calls are serialized by proper locking with problematic operations > such as hole punching etc. > > I'm not sure how to fix this problem. We could 'simulate' a writeable page > fault in set_page_dirty_lock(). It is a bit ugly since we don't have a > virtual address of the fault, don't hold mmap_sem, etc., possibly > expensive, but it would make filesystems happy. Data stored by GUP user > (e.g. read by DIO in the above case) could still get lost if someone e.g. > punched hole under the buffer or otherwise messed with the underlying > storage of the page while DIO was running but arguably users could expect > such outcome. > > Another possible solution would be to make sure page is writeably mapped > until GUP user drops its reference. That would be arguably cleaner but > probably that would mean we have to track number of writeable GUP page > references separately (no space space in struct page is a problem here) and > block page_mkclean() until they are dropped. Also for long term GUP users > like Infiniband or V4L we'd have to come up with some solution as we should > not block page_mkclean() for so long. > > As a side note DAX needs some solution for GUP users as well. The problems > are similar there in nature, just much easier to hit. So at least a > solution for long-term GUP users can (and I strongly believe should) be > shared between standard and DAX paths. > > Anybody has other ideas how to fix the problem or opinions on which > solution would be better to use or some complications I have missed? > +RDMA Hi Jan, I don't have actual proposals how to fix, but wanted to mention that we have a customer who experiences those failures in his setup. In his case, it is reproducible in 100% of cases in approximately 2 minutes of run. His application creates two memory regions with ib_umem_get(), one is backed by ext4 and another is anonymous. Approximately after two minutes of data traffic, he stops the system and calls to release those memory regions with ib_umem_release()->__ib_umem_release()->set_page_dirty_lock(). A couple of seconds later, he hits the following BUG_ON. [ 1411.545311] ------------[ cut here ]------------ [ 1411.545340] kernel BUG at fs/ext4/inode.c:2297! [ 1411.545360] invalid opcode: 0000 [#1] SMP [ 1411.545381] Modules linked in: xt_nat veth ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype iptable_filter xt_conntrack nf_nat nf_conntrack br_netfilter bridge stp llc overlay(T) rdma_ucm(OE) ib_ucm(OE) rdma_cm(OE) iw_cm(OE) ib_ipoib(OE) ib_cm(OE) ib_uverbs(OE) ib_umad(OE) mlx5_ib(OE) mlx5_core(OE) mlx4_en(OE) mlx4_ib(OE) ib_core(OE) mlx4_core(OE) devlink mlx_compat(OE) intel_powerclamp coretemp intel_rapl iosf_mbi kvm_intel vfat fat kvm irqbypass crc32_pclmul ghash_clmulni_intel aesni_intel lrw gf128mul ext4 glue_helper ablk_helper cryptd mbcache jbd2 iTCO_wdt iTCO_vendor_support mxm_wmi pcspkr sb_edac edac_core i2c_i801 sg mei_me mei shpchp lpc_ich ipmi_devintf ipmi_si ipmi_msghandler acpi_pad wmi acpi_power_meter knem(OE) nfsd auth_rpcgss [ 1411.545744] nfs_acl lockd grace sunrpc ip_tables xfs libcrc32c sd_mod crc_t10dif crct10dif_generic ast drm_kms_helper crct10dif_pclmul crct10dif_common syscopyarea sysfillrect crc32c_intel mpt3sas sysimgblt fb_sys_fops ttm ahci raid_class libahci scsi_transport_sas igb drm libata dca i2c_algo_bit ptp nvme i2c_core pps_core fjes dm_mirror dm_region_hash dm_log dm_mod [last unloaded: devlink] [ 1411.545926] CPU: 6 PID: 13195 Comm: node_runner_w8 Tainted: G W OE ------------ T 3.10.0-514.21.1.el7.debug_bz1368895.x86_64 #1 [ 1411.545975] Hardware name: Quanta Computer Inc D51BP-1U (dual 1G LoM)/S2BP-MB (dual 1G LoM), BIOS S2BP3B04 03/03/2016 [ 1411.546017] task: ffff881e4b323ec0 ti: ffff881e49bbc000 task.ti: ffff881e49bbc000 [ 1411.546047] RIP: 0010:[<ffffffffa07083e5>] [<ffffffffa07083e5>] mpage_prepare_extent_to_map+0x2d5/0x2e0 [ext4] [ 1411.546103] RSP: 0018:ffff881e49bbfc10 EFLAGS: 00010246 [ 1411.546125] RAX: 001fffff0000003d RBX: ffff881e49bbfc68 RCX: 0000000000000170 [ 1411.546154] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88207ff8dde8 [ 1411.546183] RBP: ffff881e49bbfce8 R08: 0000000000000000 R09: 0000000000000001 [ 1411.546212] R10: 57fe04c2df4f6680 R11: 0000000000000008 R12: 7ffffffffffffe9e [ 1411.546240] R13: 000000000003ffff R14: ffffea0001449a00 R15: ffff881e49bbfd90 [ 1411.546270] FS: 00007f5dd5de7d40(0000) GS:ffff881fffb80000(0000) knlGS:0000000000000000 [ 1411.546302] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1411.546325] CR2: 00007f5b0c5969c0 CR3: 0000001e5e6ae000 CR4: 00000000003407e0 [ 1411.546354] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1411.546383] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 1411.546412] Stack: [ 1411.546421] ffff881e49bbfc50 0000000000000002 ffff881f07b00628 ffff881e49bbfcb8 [ 1411.546456] 000000000000017b 000000000000000e 0000000000000000 ffffea00794f6600 [ 1411.546490] ffffea00794f6640 ffffea00794f6680 ffffea0001449a00 ffffea00014499c0 [ 1411.546524] Call Trace: [ 1411.546545] [<ffffffffa070c9ab>] ext4_writepages+0x45b/0xd60 [ext4] [ 1411.546576] [<ffffffff8118d93e>] do_writepages+0x1e/0x40 [ 1411.546601] [<ffffffff811824f5>] __filemap_fdatawrite_range+0x65/0x80 [ 1411.546629] [<ffffffff81182641>] filemap_write_and_wait_range+0x41/0x90 [ 1411.546664] [<ffffffffa0703bba>] ext4_sync_file+0xba/0x320 [ext4] [ 1411.546692] [<ffffffff8123028d>] vfs_fsync_range+0x1d/0x30 [ 1411.546717] [<ffffffff811ba89e>] SyS_msync+0x1fe/0x250 [ 1411.546741] [<ffffffff816974c9>] system_call_fastpath+0x16/0x1b [ 1411.546765] Code: ff ff ff e8 2e 7e a8 e0 8b 85 40 ff ff ff eb c2 48 8d bd 50 ff ff ff e8 1a 7e a8 e0 eb 8c 4c 89 f7 e8 a0 81 a7 e0 e9 d5 fe ff ff <0f> 0b 0f 0b e8 62 d6 97 e0 66 90 0f 1f 44 00 00 55 48 89 e5 41 [ 1411.548075] RIP [<ffffffffa07083e5>] mpage_prepare_extent_to_map+0x2d5/0x2e0 [ext4] [ 1411.549292] RSP <ffff881e49bbfc10> ---here vmcore-dmesg got cut off---- Thanks > Honza > > [1] https://www.spinics.net/lists/linux-xfs/msg10090.html > [2] https://www.spinics.net/lists/linux-ext4/msg54377.html > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-04-16 9:20 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-03 10:04 Filesystem crashes due to pages without buffers Jan Kara 2018-01-03 10:04 ` Jan Kara 2018-01-04 4:56 ` Dan Williams 2018-01-04 4:56 ` Dan Williams 2018-01-04 11:33 ` Jan Kara 2018-01-04 11:33 ` Jan Kara 2018-04-13 12:39 ` Gavin Guo 2018-04-13 12:58 ` Jan Kara 2018-04-16 9:20 ` Gavin Guo 2018-01-04 5:59 ` Dave Chinner 2018-01-04 5:59 ` Dave Chinner 2018-01-04 8:52 ` Jan Kara 2018-01-04 8:52 ` Jan Kara 2018-01-04 10:08 ` Dave Chinner 2018-01-04 10:08 ` Dave Chinner 2018-01-04 6:10 ` Leon Romanovsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.