Subject: Re: [Ext2-devel] Re: [RFC][PATCH 0/3] ext3 percpu counter fixes to suppport for ext3 unsigned long type free blocks counter - msg#00056

Previous Message by Date: click to view message preview

Re: [RFC][8/21]ext3 modify variables to exceed 2G

On Apr 13, 2006 16:06 +0900, sho@xxxxxxxxxxxxxx wrote: > Summary of this patch: > [8/21] change the type of variables manipulating a block or an > inode(ext3) > - Change the type of 4byte variables manipulating a block or > an inode from signed to unsigned. Takashi-san, please, it would make the code much more maintainable if the changes made here would use new types for filesystem-wide block offsets and for file-relative block offsets, as was previously discussed, instead of just changing some variables to be unsigned long. Like: typedef unsigned long ext3_fsblk_t; # block offset in the filesystem typedef unsigned long ext3_fscnt_t; # block count in the filesystem typedef unsigned long ext3_fileblk_t; # block offset in a file I believe we should already be using le32 for all on-disk blocks (e.g. indirect blocks, inodes, etc), but this would be good to verify. This way, when we get to the stage where we want to increase the blocks to be 64-bit (as Laurent has wanted to do) we don't need to go through and re-patch all of the same files to change "unsigned long" to "unsigned long long". It also makes the code easier to read, since it will be clear what kind of variable is being used. Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. ------------------------------------------------------- This SF.Net email is sponsored by xPML, a groundbreaking scripting language that extends applications into web and mobile media. Attend the live webcast and join the prime developer group breaking into this new coding territory! http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Next Message by Date: click to view message preview

Re: [Ext2-devel] Re: [RFC][PATCH 0/3] ext3 percpu counter fixes to suppport for ext3 unsigned long type free blocks counter

On Thu, 2006-04-13 at 12:02 -0700, Ravikiran G Thirumalai wrote: > On Wed, Apr 12, 2006 at 02:28:35PM -0700, Mingming Cao wrote: > > On Tue, 2006-04-11 at 15:20 -0700, Ravikiran G Thirumalai wrote: > > > On Tue, Apr 11, 2006 at 12:01:13PM -0700, Mingming Cao wrote: > > > > On Tue, 2006-04-11 at 10:09 -0700, Christoph Lameter wrote: > > > > > > where the check for unsigned long overflow is only turned on 32 bit > > platforms. > > > > > Or make the counter s64? so that it stays 64 bit on all arches? > > > > > > > Well, don't we have the problem : 64 bit counter add/dec/update is not > > always atomic on all 32 bit platforms? There are risk that we will get > > bogus global value. > > Yes, but the global counter is modified with a lock in the SMP case, and the > local counters are modified by their respective cpus only, Although there > might be more subtle issues ..... > Hmm, I was worried about the read to the 64 bit global counter, there is no lock to protect it from getting an half updated 64 bit counter. But I think the window is probably small, and with what Andreas suggested (keep the local per cpu counter as 32 bit value), we would avoid this non-atomic math in most cases. Well,anyway this counter is just an approximate value, and currently (with 32 bit global counter) we could still read an old global counter value while it's being updated since no lock is required while read it. > > > > > OR > > > why not change the global per-cpu counter type to unsigned long (as we > > > discussed earlier), so we don't need the extra "ul" flags and interfaces, > > > and all arches get a standard unsigned long return type? > > > We could also > > > do away with percpu_read_positive then no? The applications for per-cpu > > > counters is going to be upcounters always methinks... > > > > > > > yeah...I am not so happy with the extra "ul" checking flags either. But > > as you have mentioned before, the signed global counter type is there > > for some cases when the global counter becomes temporally negative > > ( although the counter in real life should always positive). What should > > we do if the global counter is a unsigned value, was initialized to 0, > > and now we add -5 to it(-5 is from one local counter, then we will get a > > bogus big value)? > > I thought the solution to this was to have a global unsigned counter, and > signed local counter, and defer updates to the global if it is going to be a > large value due to the case above. This way the global counter remains an up > counter no? > I think that will work, and we could get rid of the cheating percpu_counter_read_positive() totally;) So I think we could combine these two together: make the global counter an unsigned 64 bit (u64) and keep the per cpu counter still signed 32 bit type, and also, defer updates the global counter to the global if the end result is larger that before. This way we could have remove the need for percpu_counter_read_positive(), and also allow the counter being used for 64 bit accounting on 32 bit arch. Comments? Mingming > Thanks, > Kiran - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html

Previous Message by Thread: click to view message preview

Re: [Ext2-devel] Re: [RFC][PATCH 0/3] ext3 percpu counter fixes to suppport for ext3 unsigned long type free blocks counter

On Apr 12, 2006 14:28 -0700, Mingming Cao wrote: > where the check for unsigned long overflow is only turned on 32 bit > platforms. > > > Or make the counter s64? so that it stays 64 bit on all arches? > > > > Well, don't we have the problem : 64 bit counter add/dec/update is not > always atomic on all 32 bit platforms? There are risk that we will get > bogus global value. My thought here is that the per-cpu counter could still be a 32-bit counter and the global value could be a 64-bit value. That way, we don't need to mess with 64-bit math in the common case, and we can still have a 64-bit global value. The minor drawback would be that we can't have a per-cpu delta of more than 2^31 at a time, but I don't think this is a worry here. > > why not change the global per-cpu counter type to unsigned long (as we > > discussed earlier), so we don't need the extra "ul" flags and interfaces, > > and all arches get a standard unsigned long return type? > > We could also > > do away with percpu_read_positive then no? The applications for per-cpu > > counters is going to be upcounters always methinks... The "percpu_read_positive" usage is broken in any case, since it doesn't correctly handle the case where there is no space in the filesystem at all. The calling code (ext3_statfs) really needs to just call percpu_read() and then return zero if this is negative. Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html

Next Message by Thread: click to view message preview

Re: [Ext2-devel] Re: [RFC][PATCH 0/3] ext3 percpu counter fixes to suppport for ext3 unsigned long type free blocks counter

On Thu, 2006-04-13 at 12:02 -0700, Ravikiran G Thirumalai wrote: > On Wed, Apr 12, 2006 at 02:28:35PM -0700, Mingming Cao wrote: > > On Tue, 2006-04-11 at 15:20 -0700, Ravikiran G Thirumalai wrote: > > > On Tue, Apr 11, 2006 at 12:01:13PM -0700, Mingming Cao wrote: > > > > On Tue, 2006-04-11 at 10:09 -0700, Christoph Lameter wrote: > > > > > > where the check for unsigned long overflow is only turned on 32 bit > > platforms. > > > > > Or make the counter s64? so that it stays 64 bit on all arches? > > > > > > > Well, don't we have the problem : 64 bit counter add/dec/update is not > > always atomic on all 32 bit platforms? There are risk that we will get > > bogus global value. > > Yes, but the global counter is modified with a lock in the SMP case, and the > local counters are modified by their respective cpus only, Although there > might be more subtle issues ..... > Hmm, I was worried about the read to the 64 bit global counter, there is no lock to protect it from getting an half updated 64 bit counter. But I think the window is probably small, and with what Andreas suggested (keep the local per cpu counter as 32 bit value), we would avoid this non-atomic math in most cases. Well,anyway this counter is just an approximate value, and currently (with 32 bit global counter) we could still read an old global counter value while it's being updated since no lock is required while read it. > > > > > OR > > > why not change the global per-cpu counter type to unsigned long (as we > > > discussed earlier), so we don't need the extra "ul" flags and interfaces, > > > and all arches get a standard unsigned long return type? > > > We could also > > > do away with percpu_read_positive then no? The applications for per-cpu > > > counters is going to be upcounters always methinks... > > > > > > > yeah...I am not so happy with the extra "ul" checking flags either. But > > as you have mentioned before, the signed global counter type is there > > for some cases when the global counter becomes temporally negative > > ( although the counter in real life should always positive). What should > > we do if the global counter is a unsigned value, was initialized to 0, > > and now we add -5 to it(-5 is from one local counter, then we will get a > > bogus big value)? > > I thought the solution to this was to have a global unsigned counter, and > signed local counter, and defer updates to the global if it is going to be a > large value due to the case above. This way the global counter remains an up > counter no? > I think that will work, and we could get rid of the cheating percpu_counter_read_positive() totally;) So I think we could combine these two together: make the global counter an unsigned 64 bit (u64) and keep the per cpu counter still signed 32 bit type, and also, defer updates the global counter to the global if the end result is larger that before. This way we could have remove the need for percpu_counter_read_positive(), and also allow the counter being used for 64 bit accounting on 32 bit arch. Comments? Mingming > Thanks, > Kiran - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html