Use of getblk differs between locations

I've just noticed that the use of sb_getblk differs between locations inside the kernel. To be precise, in some locations there are tests against its return value, and in some places there are not.

According to the comments in __getblk definition, the tests are not necessary, as the function always return a buffer_head (maybe a wrong one),

Hi,

Quoted 7 lines · Glauber de Oliveira Costa

I've just noticed that the use of sb_getblk differs between locations inside the kernel. To be precise, in some locations there are tests against its return value, and in some places there are not.

According to the comments in __getblk definition, the tests are not necessary, as the function always return a buffer_head (maybe a wrong one),

If you had read the source code rather than just the comments you would have seen that this is not true. It can return NULL (see fs/buffer.c::__getblk_slow()). Certainly I would prefer to keep the checks in NTFS, please. They may only be good for catching bugs but I like catching bugs rather than segfaulting due to a NULL dereference.

Best regards,

Anton

Signature · 4 lines

Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Quoted 11 lines · Anton Altaparmakov

Hi,

Quoted 7 lines · Glauber de Oliveira Costa

I've just noticed that the use of sb_getblk differs between locations inside the kernel. To be precise, in some locations there are tests against its return value, and in some places there are not.

According to the comments in __getblk definition, the tests are not necessary, as the function always return a buffer_head (maybe a wrong one),

Quoted 5 lines

If you had read the source code rather than just the comments you would have seen that this is not true. It can return NULL (see fs/buffer.c::__getblk_slow()). Certainly I would prefer to keep the checks in NTFS, please. They may only be good for catching bugs but I like catching bugs rather than segfaulting due to a NULL dereference.

I did. But I did not see this specifically, for sure. What takes us to the opposite problem: A lot of places do not check for the return value of getblk (Almost half of them, I'd say), and may thus lead to a dereferencing of a NULL pointer.

Does anyone else have any comments on that?

Quoted 1 line

Best regards,

Thanks,

Quoted 1 line

Anton

Glauber

Quoted 6 lines

--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @) Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Signature · 5 lines

=====================================
Glauber de Oliveira Costa
IBM Linux Technology Center - Brazil
[email protected]
=====================================

Quoted 10 lines · Glauber de Oliveira Costa

Quoted 9 lines · Anton Altaparmakov

Quoted 7 lines · Glauber de Oliveira Costa

I've just noticed that the use of sb_getblk differs between locations inside the kernel. To be precise, in some locations there are tests against its return value, and in some places there are not.

According to the comments in __getblk definition, the tests are not necessary, as the function always return a buffer_head (maybe a wrong one),

Quoted 5 lines

Quoted 5 lines

If you had read the source code rather than just the comments you would have seen that this is not true. It can return NULL (see fs/buffer.c::__getblk_slow()). Certainly I would prefer to keep the checks in NTFS, please. They may only be good for catching bugs but I like catching bugs rather than segfaulting due to a NULL dereference.

The check should be rather a BUG() than dump_stack() and return NULL --- I think it's not right to write code to recover from programming errors.

Filesystem drivers are supposed to pass correct blocksize to getblk(). --- even for users it's better to crash, because user whose machine has locked up on BUG() will report bug more likely than user whose machine has written stack dump into log and corrupted filesystem --- by the time he discovers the corruption and mesage he might not even remember what triggered it.

As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

Quoted 17 lines · Mikulas Patocka

Quoted 15 lines · Glauber de Oliveira Costa

Quoted 14 lines · Anton Altaparmakov

Quoted 7 lines · Glauber de Oliveira Costa

I've just noticed that the use of sb_getblk differs between locations inside the kernel. To be precise, in some locations there are tests against its return value, and in some places there are not.

According to the comments in __getblk definition, the tests are not necessary, as the function always return a buffer_head (maybe a wrong one),

If you had read the source code rather than just the comments you would have seen that this is not true. It can return NULL (see fs/buffer.c::__getblk_slow()). Certainly I would prefer to keep the checks in NTFS, please. They may only be good for catching bugs but I like catching bugs rather than segfaulting due to a NULL dereference.

Quoted 2 lines

The check should be rather a BUG() than dump_stack() and return NULL --- I think it's not right to write code to recover from programming errors.

Why programming errors? It could be faulty memory or other corruption, perhaps even caused by a different driver altogether (e.g. I found a bug in ntfs last week which caused it to memset() to zero a random location in memory of a random size and it caused a lot of strange effects like my

shell suddenly exiting and me being left on the login prompt...). Also it could be that the function one day changes and it can return NULL. It is far safer to do checking than to make assumptions about not being able to return NULL.

Quoted 6 lines

Filesystem drivers are supposed to pass correct blocksize to getblk(). --- even for users it's better to crash, because user whose machine has locked up on BUG() will report bug more likely than user whose machine has written stack dump into log and corrupted filesystem --- by the time he discovers the corruption and mesage he might not even remember what triggered it.

Quoted 3 lines

As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

What do you mean corrupt filesystem? If a filesystem is written so badly that it will cause corruption when a NULL is returned somewhere, I certainly don't want to have anything to do with it.

Going BUG() is generally a bad thing if the error can be recovered from. Certainly all my code attempts to recover from all error conditions it can possibly encounter.

I would much rather see NULL and then handle the error gracefully with an error message than go BUG(). You can then still umount and remove the fs module and everything works fine (you may need an fsck you may not depends on how good your error handling is). If you do a BUG() you are guaranteed to cause corruption...

I only use BUG() when something really cannot happen unless there is a bug in which case I want to know it...

Best regards,

Anton

Signature · 4 lines

Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Quoted 4 lines

Going BUG() is generally a bad thing if the error can be recovered from. Certainly all my code attempts to recover from all error conditions it can possibly encounter.

Quoted 8 lines

I would much rather see NULL and then handle the error gracefully with an error message than go BUG(). You can then still umount and remove the fs module and everything works fine (you may need an fsck you may not depends on how good your error handling is). If you do a BUG() you are guaranteed to cause corruption...

I only use BUG() when something really cannot happen unless there is a bug in which case I want to know it...

Of course, this "can't happen unless there is a bug" is exactly the case of __getblk_slow().

Mikulas

Quoted 9 lines

Best regards,

Anton

Signature · 4 lines

Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Quoted 3 lines

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Maybe stopping gracefully, warn about what happened, and let the system keep going. You may be right about your main filesystem, but in the case I'm running, for example, my system in an ext3 filesystem, and have a vfat from a usb key. Should my system really hang because I'm not able to read/write to the device?

Quoted 9 lines · Glauber de Oliveira Costa

Quoted 3 lines

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Maybe stopping gracefully, warn about what happened, and let the system keep going. You may be right about your main filesystem, but in the case I'm running, for example, my system in an ext3 filesystem, and have a vfat from a usb key. Should my system really hang because I'm not able to read/write to the device?

getblk won't fail because of I/O error --- it can fail only because of memory management bugs. I think it's right to stop the system in that case --- it's better than silently corrupting data on any device.

Mikulas

Quoted 9 lines

Quoted 8 lines

Quoted 2 lines

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Maybe stopping gracefully, warn about what happened, and let the system keep going. You may be right about your main filesystem, but in the case I'm running, for example, my system in an ext3 filesystem, and have a vfat from a usb key. Should my system really hang because I'm not able to read/write to the device?

Quoted 6 lines

getblk won't fail because of I/O error --- it can fail only because of memory management bugs. I think it's right to stop the system in that case --- it's better than silently corrupting data on any device.

Mikulas

In the code, we see:

if (unlikely(size & (bdev_hardsect_size(bdev)-1) ||

                       (size < 512 || size > PAGE_SIZE))) {

This is where __getblk_slow, and thus, __getblk fails, and it does not seem to be due to any memory management bug.

Quoted 5 lines

In the code, we see:

if (unlikely(size & (bdev_hardsect_size(bdev)-1) ||

                      (size < 512 || size > PAGE_SIZE))) {

Quoted 2 lines

This is where __getblk_slow, and thus, __getblk fails, and it does not seem to be due to any memory management bug.

This is a filesystem bug --- filesystem should set it's blocksize with sb_set_blocksize (and refuse to mount if the device doesn't support it) before using it in requests.

Mikulas

Quoted 6 lines · Anton Altaparmakov

Quoted 4 lines · Mikulas Patocka

On Mon, 10 Oct 2005, Glauber de Oliveira Costa wrote:
As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

Quoted 3 lines

What do you mean corrupt filesystem? If a filesystem is written so badly that it will cause corruption when a NULL is returned somewhere, I certainly don't want to have anything to do with it.

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Quoted 11 lines · Mikulas Patocka

Quoted 8 lines · Anton Altaparmakov

Quoted 4 lines · Mikulas Patocka

On Mon, 10 Oct 2005, Glauber de Oliveira Costa wrote:
As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

What do you mean corrupt filesystem? If a filesystem is written so badly that it will cause corruption when a NULL is returned somewhere, I certainly don't want to have anything to do with it.

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Two clear choices:

1) Switch to read-only and use the cached data to fulfil requests and fail all others.

2) Ask the user to insert the media/plug the device back in (this is by far the most likely cause of all requests suddenly failing) and then continue where they left off.

It is unfortunate that Linux does not allow for 2) so you need to do 1).

Quoted 7 lines · Anton Altaparmakov

Quoted 3 lines · Mikulas Patocka

On Mon, 10 Oct 2005, Anton Altaparmakov wrote:
What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Two clear choices:

Quoted 8 lines

1) Switch to read-only and use the cached data to fulfil requests and fail all others.

2) Ask the user to insert the media/plug the device back in (this is by far the most likely cause of all requests suddenly failing) and then continue where they left off.

It is unfortunate that Linux does not allow for 2) so you need to do 1).

I recently looked into 2) a bit and the dm multipath code is almost enough to do exactly this.

If you configure your block device as an mpath device that queues on path failure, and change the table to the new device location on device re-attach, the queued up i/o will be flushed out. Almost. Right now, when you change the table and resume the dm mapping, it does a suspend which attempts to write out the data to a device which is no longer there, causing it to just be dropped on the floor. If this were changed not to do that, and perhaps set a timer so that the dirty data wouldn't be left around forever if the device wasn't reattached, 2) would definitely be possible.

I realize that the userspace intervention required may involve a bit of dark magic, but my point is most of the code required on the kernel side is already implemented.

- -Jeff

Quoted 31 lines

Quoted 8 lines · Anton Altaparmakov

Quoted 4 lines · Mikulas Patocka

On Mon, 10 Oct 2005, Anton Altaparmakov wrote:
What should a filesystem driver do if it can't suddenly read or write any
blocks on media?

Two clear choices:

Quoted 8 lines

1) Switch to read-only and use the cached data to fulfil requests and fail all others.

2) Ask the user to insert the media/plug the device back in (this is by far the most likely cause of all requests suddenly failing) and then continue where they left off.

It is unfortunate that Linux does not allow for 2) so you need to do 1).

I recently looked into 2) a bit and the dm multipath code is almost enough to do exactly this.

If you configure your block device as an mpath device that queues on path failure, and change the table to the new device location on device re-attach, the queued up i/o will be flushed out. Almost. Right now, when you change the table and resume the dm mapping, it does a suspend which attempts to write out the data to a device which is no longer there, causing it to just be dropped on the floor. If this were changed not to do that, and perhaps set a timer so that the dirty data wouldn't be left around forever if the device wasn't reattached, 2) would definitely be possible.

Quoted 5 lines

I realize that the userspace intervention required may involve a bit of dark magic, but my point is most of the code required on the kernel side is already implemented.

- -Jeff

Is memory management ready for this? Can't deadlock like this happen?
- displaying dialog window needs memory, so it waits until memory will be available
- system decides to write some write-back cached data in order to free memory
- the write of these data waits until the dialog window is displayed, user inserts the device and clicks 'OK'

Quoted 7 lines

Is memory management ready for this? Can't deadlock like this happen?
- displaying dialog window needs memory, so it waits until memory will be available
- system decides to write some write-back cached data in order to free memory
- the write of these data waits until the dialog window is displayed, user inserts the device and clicks 'OK'

No, it's not, and deadlock is definitely possible. However, if we're at the point where memory is tight enough that it's an issue, the timer can expire and all the pending i/o is dropped just as it would be without the multipath code enabled.

I'm not saying it's a solution ready for production, just a good starting point.

Quoted 13 lines

Quoted 7 lines

Is memory management ready for this? Can't deadlock like this happen?
- displaying dialog window needs memory, so it waits until memory will be available
- system decides to write some write-back cached data in order to free memory
- the write of these data waits until the dialog window is displayed, user inserts the device and clicks 'OK'

No, it's not, and deadlock is definitely possible. However, if we're at the point where memory is tight enough that it's an issue, the timer can expire and all the pending i/o is dropped just as it would be without the multipath code enabled.

Quoted 2 lines

I'm not saying it's a solution ready for production, just a good starting point.

But discarding data sometimes on USB unplug is even worse than discarding data always --- users will by experimenting learn that linux doesn't discard write-cached data and reminds them to replug the device --- and one day, randomly, they lose their data because of some memory management condition...

Quoted 4 lines

But discarding data sometimes on USB unplug is even worse than discarding data always --- users will by experimenting learn that linux doesn't discard write-cached data and reminds them to replug the device --- and one day, randomly, they lose their data because of some memory management condition...

And how exactly is that worse than discarding the data every time?!?!?!?

Best regards,

Anton

Quoted 8 lines

Quoted 6 lines

But discarding data sometimes on USB unplug is even worse than discarding data
always --- users will by experimenting learn that linux doesn't discard write-cached data and reminds them to replug the device --- and one day, randomly, they lose their data because of some memory management condition...

And how exactly is that worse than discarding the data every time?!?!?!?

Undeterministic behaviour is worse than deterministic. You can learn the system that behaves deterministically.

If you know that unplug damages filesystem on your USB disk, you replug it, recheck filesystem and copy the important data again --- you have 0% probability of data damage.
However, if damage on unplug happens only with 1/100 probability, will you still check filesystem and copy all recently created files on it? You forget it (or you wouldn't even know that damage might occur) and you have 1% probability of data damage.

Quoted 8 lines

Quoted 6 lines

But discarding data sometimes on USB unplug is even worse than discarding data
always --- users will by experimenting learn that linux doesn't discard write-cached data and reminds them to replug the device --- and one day, randomly, they lose their data because of some memory management condition...

And how exactly is that worse than discarding the data every time?!?!?!?

Undeterministic behaviour is worse than deterministic. You can learn the system that behaves deterministically.

If you know that unplug damages filesystem on your USB disk, you replug it, recheck filesystem and copy the important data again --- you have 0% probability of data damage.
However, if damage on unplug happens only with 1/100 probability, will you still check filesystem and copy all recently created files on it? You forget it (or you wouldn't even know that damage might occur) and you have 1% probability of data damage.

You cannot design a sane system with sane performance that cannot OOM. And when that happens applications will get killed so you will loose all your data anyway.
So who cares that some data on the usb stick may then be lost?

But discarding data sometimes on USB unplug is even worse than discarding data always --- users will by experimenting learn that linux doesn't discard write-cached data and reminds them to replug the device --- and one day, randomly, they lose their data because of some memory management condition...

Quoted 5 lines · Mikulas Patocka

But discarding data sometimes on USB unplug is even worse than discarding data always --- users will by experimenting learn that linux doesn't discard write-cached data and reminds them to replug the device --- and one day, randomly, they lose their data because of some memory management condition...

It should not happen provided the total amount of dirty data for detachable devices is restricted to allow enough room for opening a dialog.

That's no different, in principle, than the restrictions that are used to ensure some types of kernel memory allocation always succeed.

There's no exact calculation, just a notion of "this many megabytes should be enough for a dialog".

-- Jamie

That is possible ... you must also make sure that you do not hold an important semaphore while waiting for some removable device (auditing VFS for this will be a bit harder...)

Quoted 11 lines

Quoted 3 lines

- the write of these data waits until the dialog window is displayed,
user inserts the device and clicks 'OK'

No, it's not, and deadlock is definitely possible. However, if we're at
the point where memory is tight enough that it's an issue, the timer can
expire and all the pending i/o is dropped just as it would be without the multipath code enabled.

Quoted 2 lines

I'm not saying it's a solution ready for production, just a good starting point.

But discarding data sometimes on USB unplug is even worse than discarding data always --- users will by experimenting learn that

*Good starting point*.

Anyway, one solution would be to simply mlockall() on that replugitd

and/or make dirty data hdd based (not ram based)

and/or restricting dirty buffers to 10MB for removable media.

Quoted 1 line · Pavel Machek

and/or make dirty data hdd based (not ram based)

Ooh, swappable dirty data... nice idea :)

Quoted 1 line

and/or restricting dirty buffers to 10MB for removable media.

That seems like the simplest effective solution.

-- Jamie

No, it's not, and deadlock is definitely possible. However, if we're at the point where memory is tight enough that it's an issue, the timer can expire and all the pending i/o is dropped just as it would be without the multipath code enabled.

Quoted 15 lines · Anton Altaparmakov

Quoted 11 lines · Mikulas Patocka

Quoted 8 lines · Anton Altaparmakov

Quoted 4 lines · Mikulas Patocka

On Mon, 10 Oct 2005, Glauber de Oliveira Costa wrote:
As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

What do you mean corrupt filesystem? If a filesystem is written so badly that it will cause corruption when a NULL is returned somewhere, I certainly don't want to have anything to do with it.

What should a filesystem driver do if it can't suddenly read or write any blocks on media?

Two clear choices:

Quoted 8 lines

1) Switch to read-only and use the cached data to fulfil requests and fail all others.

2) Ask the user to insert the media/plug the device back in (this is by far the most likely cause of all requests suddenly failing) and then continue where they left off.

It is unfortunate that Linux does not allow for 2) so you need to do 1).

I recently looked into 2) a bit and the dm multipath code is almost enough to do exactly this.

If you configure your block device as an mpath device that queues on path failure, and change the table to the new device location on device re-attach, the queued up i/o will be flushed out. Almost. Right now, when you change the table and resume the dm mapping, it does a suspend which attempts to write out the data to a device which is no longer there, causing it to just be dropped on the floor. If this were changed not to do that, and perhaps set a timer so that the dirty data wouldn't be left around forever if the device wasn't reattached, 2) would definitely be possible.

I realize that the userspace intervention required may involve a bit of dark magic, but my point is most of the code required on the kernel side is already implemented.

Quoted 5 lines

Quoted 5 lines

If you had read the source code rather than just the comments you would have seen that this is not true. It can return NULL (see fs/buffer.c::__getblk_slow()). Certainly I would prefer to keep the checks in NTFS, please. They may only be good for catching bugs but I like catching bugs rather than segfaulting due to a NULL dereference.

The check should be rather a BUG() than dump_stack() and return NULL --- I think it's not right to write code to recover from programming errors. Filesystem drivers are supposed to pass correct blocksize to getblk(). --- even for users it's better to crash, because user whose machine has locked up on BUG() will report bug more likely than user whose machine has written stack dump into log and corrupted filesystem --- by the time he discovers the corruption and mesage he might not even remember what triggered it.

As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

Mikulas

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

If you had read the source code rather than just the comments you would have seen that this is not true. It can return NULL (see fs/buffer.c::__getblk_slow()). Certainly I would prefer to keep the checks in NTFS, please. They may only be good for catching bugs but I like catching bugs rather than segfaulting due to a NULL dereference.

Quoted 8 lines

The check should be rather a BUG() than dump_stack() and return NULL --- I think it's not right to write code to recover from programming errors. Filesystem drivers are supposed to pass correct blocksize to getblk(). --- even for users it's better to crash, because user whose machine has locked up on BUG() will report bug more likely than user whose machine has written stack dump into log and corrupted filesystem --- by the time he discovers the corruption and mesage he might not even remember what triggered it.

That was what I meant by having the opposite problem here. I think dumping the stack and returning NULL is okay, as long as all programmers test its return value, and decide to fail in an alternative way, just like Anton does, for example. But unfortunately, that's not what happen.

In a lot of cases, we see uses like these: (This one from affs.h)

	bh = sb_getblk(sb, block);
	lock_buffer(bh);
	memset(bh->b_data, 0 , sb->s_blocksize);
	set_buffer_uptodate(bh);
	unlock_buffer(bh);

Which does not seem to be the right usage for it.

As I said, I took away the checks because I missed that return statement. I usually don't think that hanging is the preferred solution in the cases in which you can stop gracefully - But in case you do stop gracefully, not dereference a NULL pointer.

Quoted 5 lines

As comment in buffer.c says, getblk will deadlock if the machine is out of memory. It is questionable whether to deadlock or return NULL and corrupt filesystem in this case --- deadlock is probably better.

Quoted 1 line

Mikulas

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

What do you think?

Quoted 5 lines

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

What do you think?

I certainly agree with you there. I neither want a deadlock nor corruption. (-:

Quoted 8 lines · Anton Altaparmakov

Quoted 5 lines

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

What do you think?

I certainly agree with you there. I neither want a deadlock nor corruption. (-:

Yup. In the present implementation __getblk_slow() "cannot fail". It's conceivable that at some future stage we'll change __getblk_slow() so that it returns NULL on an out-of-memory condition. Anyone making such a change would have to audit all callers to make sure that they handle the NULL correctly.

It is appropriate at this time to fix the callers so that they correctly handle the NULL return. However, it is non-trivial to actually _test_ such changes, and such changes should be tested. Or at least, they should be done with considerable care and knowledge of the specific filesystems.

fs/bfs/file.c static int bfs_move_block(unsigned long · line 37

Quoted 5 lines · Glauber de Oliveira Costa

fs/bfs/file.c +2 -0

 	if (!bh)
 		return -EIO;
 	new = sb_getblk(sb, to);
+	if (!new)
+		return -EIO;

You've just introduced a leak here, obviously.

Please, read the code before "fixing" that stuff; slapping returns at random and hoping that it will help is not a good way to deal with that - the only thing you achieve is hiding the problem.

The same goes for the rest of patch - in each case it's not obvious that your changes are correct.

[...]
In the code you commented, I thought that we get the same case testing from or to conditions, and thus, it would be correct to threat them in the same way.

[...]

Quoted 9 lines · Al Viro

fs/bfs/file.c static int bfs_move_block(unsigned long · line 37

Quoted 5 lines · Glauber de Oliveira Costa

fs/bfs/file.c +2 -0

 	if (!bh)
 		return -EIO;
 	new = sb_getblk(sb, to);
+	if (!new)
+		return -EIO;

You've just introduced a leak here, obviously.

Quoted 6 lines

Please, read the code before "fixing" that stuff; slapping returns at random and hoping that it will help is not a good way to deal with that - the only thing you achieve is hiding the problem.

The same goes for the rest of patch - in each case it's not obvious that your changes are correct.

Quoted 4 lines · Anton Altaparmakov

Quoted 4 lines

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

Quoted 4 lines

Quoted 1 line

What do you think?

I certainly agree with you there. I neither want a deadlock nor corruption. (-:

Yup. In the present implementation __getblk_slow() "cannot fail". It's conceivable that at some future stage we'll change __getblk_slow() so that it returns NULL on an out-of-memory condition.

Anyone making such a change
would have to audit all callers to make sure that they handle the NULL correctly.

It is appropriate at this time to fix the callers so that they correctly handle the NULL return. However, it is non-trivial to actually _test_ such changes, and such changes should be tested. Or at least, they should be done with considerable care and knowledge of the specific filesystems.

Quoted 10 lines · Andrew Morton

Quoted 8 lines · Anton Altaparmakov

Quoted 5 lines

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

What do you think?

I certainly agree with you there. I neither want a deadlock nor corruption. (-:

Quoted 3 lines

Yup. In the present implementation __getblk_slow() "cannot fail". It's conceivable that at some future stage we'll change __getblk_slow() so that it returns NULL on an out-of-memory condition.

The question is if it is desired --- it will make bread return NULL on out-of-memory condition, callers will treat it like an IO error, skipping access to the affected block, causing damage on perfectly healthy filesystem.

I liked what linux-2.0 did in this case --- if the kernel was out of memory, getblk just took another buffer, wrote it if it was dirty and used it. Except for writeable loopback device (where writing one buffer generates more dirty buffers), it couldn't deadlock.

Quoted 4 lines

I liked what linux-2.0 did in this case --- if the kernel was out of memory, getblk just took another buffer, wrote it if it was dirty and used it. Except for writeable loopback device (where writing one buffer generates more dirty buffers), it couldn't deadlock.

Wouldn't it be better if bread() were to return ERR_PTR(-EIO) or
ERR_PTR(-ENOMEM)? Big change.

Quoted 8 lines

Quoted 5 lines

I liked what linux-2.0 did in this case --- if the kernel was out of memory, getblk just took another buffer, wrote it if it was dirty and used it. Except for writeable loopback device (where writing one buffer generates more dirty buffers), it couldn't deadlock.

Wouldn't it be better if bread() were to return ERR_PTR(-EIO) or
ERR_PTR(-ENOMEM)? Big change.

No. Out of memory condition can happen even under normal circumstances under lightly loaded system. Think of a situation when dirty file-mapped pages fill up the whole memory, now a burst of packets from network comes that fills up kernel atomic reserve, you have zero pages free --- and what now? --- returning ENOMEM and dropping dirty pages without writing them is wrong, deadlocking (filesystem waits until memory manager frees some pages and memory manager waits until filesystem writes the dirty pages) is wrong too.

Quoted 7 lines

Quoted 4 lines

I liked what linux-2.0 did in this case --- if the kernel was out of memory, getblk just took another buffer, wrote it if it was dirty and used it. Except for writeable loopback device (where writing one buffer generates more dirty buffers), it couldn't deadlock.

Wouldn't it be better if bread() were to return ERR_PTR(-EIO) or
ERR_PTR(-ENOMEM)? Big change.

No. Out of memory condition can happen even under normal circumstances under lightly loaded system. Think of a situation when dirty file-mapped pages fill up the whole memory, now a burst of packets from network comes that fills up kernel atomic reserve, you have zero pages free --- and what now? --- returning ENOMEM and dropping dirty pages without writing them is wrong,

deadlocking (filesystem waits until memory manager frees some pages
and memory manager waits until filesystem writes the dirty pages) is wrong too.

The filesystem must make sure that it doesn't need any memory to do block allocation and data write. Linux-2.0 got this right, it could do getblk and bread even if get_free_pages constantly failed.

Quoted 13 lines · Mikulas Patocka

Quoted 10 lines · Andrew Morton

Quoted 8 lines · Anton Altaparmakov

Quoted 5 lines

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

What do you think?

I certainly agree with you there. I neither want a deadlock nor corruption. (-:

Quoted 8 lines

Quoted 3 lines

Yup. In the present implementation __getblk_slow() "cannot fail". It's conceivable that at some future stage we'll change __getblk_slow() so that it returns NULL on an out-of-memory condition.

The question is if it is desired --- it will make bread return NULL on out-of-memory condition, callers will treat it like an IO error, skipping access to the affected block, causing damage on perfectly healthy filesystem.

Yes, that is a bit dumb. A filesystem might indeed want to take different action for ENOMEM versus EIO.

Quoted 4 lines

I liked what linux-2.0 did in this case --- if the kernel was out of memory, getblk just took another buffer, wrote it if it was dirty and used it. Except for writeable loopback device (where writing one buffer generates more dirty buffers), it couldn't deadlock.

Wouldn't it be better if bread() were to return ERR_PTR(-EIO) or
ERR_PTR(-ENOMEM)? Big change.

Quoted 26 lines · Andrew Morton

Quoted 9 lines · Mikulas Patocka

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Anton Altaparmakov

Quoted 5 lines

Maybe the best solution is neither one nor another. Testing and failing gracefully seems better.

What do you think?

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

I certainly agree with you there. I neither want a deadlock nor corruption. (-:

Yup. In the present implementation __getblk_slow() "cannot fail". It's conceivable that at some future stage we'll change __getblk_slow() so that it returns NULL on an out-of-memory condition.

The question is if it is desired --- it will make bread return NULL on out-of-memory condition, callers will treat it like an IO error, skipping access to the affected block, causing damage on perfectly healthy filesystem.

Yes, that is a bit dumb. A filesystem might indeed want to take different action for ENOMEM versus EIO.

Quoted 1 line

I liked what linux-2.0 did in this case --- if the kernel was out of memory, getblk just took another buffer, wrote it if it was dirty and used it. Except for writeable loopback device (where writing one buffer generates more dirty buffers), it couldn't deadlock.

Quoted 2 lines

Wouldn't it be better if bread() were to return ERR_PTR(-EIO) or
ERR_PTR(-ENOMEM)? Big change.

It would indeed. Much better. And whilst at it, it would be even better if we had a lot more error codes like "ERR_PTR(-EDEVUNPLUGGED)" for example... But that would be an even better change. Anyone feeling like touching every block driver in the kernel? (-;